Skip to content

Conversation

@jdye64
Copy link
Collaborator

@jdye64 jdye64 commented Nov 13, 2021

This PR modifies the parser configuration to ensure that case does not matter across queries. This closes #315

@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2021

Codecov Report

Merging #316 (7ab43d9) into main (96524ee) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #316      +/-   ##
==========================================
+ Coverage   95.69%   95.70%   +0.01%     
==========================================
  Files          65       65              
  Lines        2854     2864      +10     
  Branches      534      536       +2     
==========================================
+ Hits         2731     2741      +10     
+ Misses         75       74       -1     
- Partials       48       49       +1     
Impacted Files Coverage Δ
dask_sql/context.py 99.13% <100.00%> (+<0.01%) ⬆️
dask_sql/physical/rel/custom/create_model.py 93.22% <0.00%> (-2.94%) ⬇️
dask_sql/physical/utils/sort.py 90.62% <0.00%> (+7.29%) ⬆️

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 96524ee...7ab43d9. Read the comment docs.

@jdye64
Copy link
Collaborator Author

jdye64 commented Nov 13, 2021

One of the tests timed out. Running again. rerun tests

@ayushdg
Copy link
Collaborator

ayushdg commented Nov 15, 2021

Do you think that this could use the config options in #286 to expose this as something that can be exposed from the python layer to the java layer. Config options doesn't do this today but other parts of the code such as create_model or create_experiment allow this. The only difference in those cases is that they are implemented as a custom plugin. I'm not sure how the options could be piped to an existing plugin

@jdye64
Copy link
Collaborator Author

jdye64 commented Nov 19, 2021

Do you think that this could use the config options in #286 to expose this as something that can be exposed from the python layer to the java layer. Config options doesn't do this today but other parts of the code such as create_model or create_experiment allow this. The only difference in those cases is that they are implemented as a custom plugin. I'm not sure how the options could be piped to an existing plugin

Sure, I just made the needed changes for that to happen. I just hard coded it to False for now but once #286 merges I will make another update to use that higher ConfigurationContainer abstraction.

# Now create a relational algebra from that
generator_builder = RelationalAlgebraGeneratorBuilder(self.schema_name)
ignore_case = self.schema[self.schema_name].config.get_config_by_prefix(
"dask.sql.identifier.ignore_case"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am following existing logic here using magic strings but think we should likely move all of the configuration keys to mappings.py and add further documentation in a subsequent PR. I am happy to make that PR as well once this one merges. Would prefer to not make that change in this PR since that change effects other codebase areas as well.

@jdye64
Copy link
Collaborator Author

jdye64 commented Dec 7, 2021

rerun tests

@jdye64
Copy link
Collaborator Author

jdye64 commented Dec 7, 2021

@charlesbluca not sure what is up with CI but seems like everything passed??

@charlesbluca
Copy link
Collaborator

Looks like gpuCI got hung up on the Docker image pull and timed out; there was an issue opened about this recently in Dask: dask/dask#8465

rerun tests

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.

Thanks for the work here @jdye64 😄 just one last comment:

@jdye64
Copy link
Collaborator Author

jdye64 commented Dec 7, 2021

rerun tests

1 similar comment
@charlesbluca
Copy link
Collaborator

rerun tests

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.

Thanks for the work here @jdye64! Think the gpuCI failures yesterday may have been related to the AWS outage

@charlesbluca charlesbluca merged commit 2640a8e into dask-contrib:main Dec 8, 2021
@charlesbluca charlesbluca mentioned this pull request Dec 8, 2021
@jdye64 jdye64 deleted the case_sensitivity branch December 21, 2021 20:40
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.

[ENH] Support user control over case sensitivity

4 participants