Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve doc on lowercase treatment of columns on SQL #3385

Merged
merged 5 commits into from
Oct 12, 2022

Conversation

nanicpc
Copy link
Contributor

@nanicpc nanicpc commented Sep 7, 2022

Adds specific note on lowercase treatment of columns on SQL

Which issue does this PR close?

Closes #2374

Rationale for this change

This example is the first thing newcomers see when they start with datafusion (literally the second page on the docs) so it should be clearer.

What changes are included in this PR?

Improvements on the Documentation:

  • the "Example Usage" case
  • the SELECT page of SQL

Adds specific note on lowercase treatment of columns on SQL

This example is the first thing newcomers see when they start with datafusion (literally the second page on the docs) so it should be clearer.
add note on the use of default lower-case for SQL query
@alamb alamb changed the title Improve doc Improve doc on lowercase treatment of columns on SQL Sep 7, 2022
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @nanicpc -- I think the notes are great. I am not sure about the change to the example code.

docs/source/user-guide/example-usage.md Show resolved Hide resolved
@@ -19,6 +19,8 @@

# Example Usage

In this example some simple processing is performed on a csv file. Please be aware that all identifiers are made lower-case in SQL, so if your csv file has capital letters (ex: Name) you should put your column name in double quotes or the example won't work.
Copy link
Contributor

Choose a reason for hiding this comment

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

This note is great

@codecov-commenter
Copy link

Codecov Report

Merging #3385 (21487dd) into master (d16457a) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 21487dd differs from pull request most recent head d39cbdd. Consider uploading reports for the commit d39cbdd to get more accurate results

@@           Coverage Diff           @@
##           master    #3385   +/-   ##
=======================================
  Coverage   85.58%   85.58%           
=======================================
  Files         296      296           
  Lines       54175    54187   +12     
=======================================
+ Hits        46364    46377   +13     
+ Misses       7811     7810    -1     
Impacted Files Coverage Δ
...fusion/optimizer/src/single_distinct_to_groupby.rs 98.33% <0.00%> (+0.11%) ⬆️
datafusion/expr/src/logical_plan/plan.rs 77.61% <0.00%> (+0.16%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@alamb
Copy link
Contributor

alamb commented Sep 12, 2022

Marking as draft to signify this PR has planned changes (and make it easier to find PRs that are in need of review). Please mark "ready for review" when it is next ready or if this change was a mistake.

@alamb alamb marked this pull request as draft September 12, 2022 11:05
@nanicpc nanicpc marked this pull request as ready for review October 11, 2022 09:29
@github-actions github-actions bot added the core Core DataFusion crate label Oct 11, 2022
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @nanicpc

@alamb
Copy link
Contributor

alamb commented Oct 11, 2022

What do you think about changing the introductory example in the user guide @andygrove ? Given it is the first thing some people see I think there is a potential reason to keep it as simple as possible (and thus maybe it would be nice to leave it uncapitalized, and add add a link about the important subtlety of capitalization to somewhere later?

If you agree I will merge this PR and then make a follow on PR that tries to retain the simple initial example as well as the capitalized example

@andygrove
Copy link
Member

What do you think about changing the introductory example in the user guide @andygrove ? Given it is the first thing some people see I think there is a potential reason to keep it as simple as possible (and thus maybe it would be nice to leave it uncapitalized, and add add a link about the important subtlety of capitalization to somewhere later?

If you agree I will merge this PR and then make a follow on PR that tries to retain the simple initial example as well as the capitalized example

That sounds great. Thanks @alamb

@alamb alamb merged commit d5e6736 into apache:master Oct 12, 2022
@alamb
Copy link
Contributor

alamb commented Oct 12, 2022

Thanks again @nanicpc

@ursabot
Copy link

ursabot commented Oct 12, 2022

Benchmark runs are scheduled for baseline = d5c361b and contender = d5e6736. d5e6736 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@alamb
Copy link
Contributor

alamb commented Oct 12, 2022

Follow on PR in #3815

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify in docs that Identifiers are made lower-case in SQL query
5 participants