Skip to content

Conversation

@ayushdg
Copy link
Collaborator

@ayushdg ayushdg commented Jan 19, 2022

As discussed in #258 it seems like SUBSTR is indeed supported and the error was most likely due to case sensitivity.

This PR adds a couple of test cases for SUBSTR since only SUBSTRING was covered.

Also a minor change that replaces the use of now deprecated distutils.version.LooseVersion with packaging.version.Version

Copy link
Collaborator

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; thanks for adding these tests @ayushdg 😄

Test failures seem to be because of the breakage in dask-ml

@codecov-commenter
Copy link

codecov-commenter commented Jan 19, 2022

Codecov Report

Merging #372 (420ffa9) into main (408f976) will decrease coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #372      +/-   ##
==========================================
- Coverage   95.57%   95.46%   -0.11%     
==========================================
  Files          67       67              
  Lines        2935     2932       -3     
  Branches      547      546       -1     
==========================================
- Hits         2805     2799       -6     
- Misses         79       83       +4     
+ Partials       51       50       -1     
Impacted Files Coverage Δ
dask_sql/_compat.py 100.00% <100.00%> (ø)
dask_sql/physical/rel/custom/show_models.py 53.33% <0.00%> (-26.67%) ⬇️
dask_sql/physical/utils/sort.py 93.10% <0.00%> (+2.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 408f976...420ffa9. Read the comment docs.

@charlesbluca charlesbluca merged commit ec97bee into dask-contrib:main Jan 24, 2022
@ayushdg ayushdg deleted the add-substr-tests branch February 6, 2023 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants