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

SNOW-1748403: Use Aggregate.aggregate_expressions to infer quoted identifiers #2526

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

sfc-gh-jdu
Copy link
Collaborator

  1. Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-1748403

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
      • If this test skips Local Testing mode, I'm requesting review from @snowflakedb/local-testing
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am adding new credentials
    • I am adding a new dependency
    • If this is a new feature/behavior, I'm adding the Local Testing parity changes.
    • I acknowledge that I have ensured my changes to be thread-safe. Follow the link for more information: Thread-safe Developer Guidelines
  3. Please describe how your code solves the related issue.

    We can use Aggregare.aggregate_expressions directly for quoted identifiers

@sfc-gh-jdu sfc-gh-jdu added the NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md label Oct 28, 2024
@sfc-gh-jdu sfc-gh-jdu requested review from a team as code owners October 28, 2024 22:27
@sfc-gh-jdu sfc-gh-jdu changed the title SNOW-1748403: Use Aggregare.aggregate_expressions as cached quoted identifiers SNOW-1748403: Use Aggregare.aggregate_expressions to infer quoted identifiers Oct 28, 2024
@sfc-gh-helmeleegy
Copy link
Contributor

This looks great. Is there a way to quantify the impact of each of those improvements on the reduction of describe queries?

@sfc-gh-jdu
Copy link
Collaborator Author

This looks great. Is there a way to quantify the impact of each of those improvements on the reduction of describe queries?

Yeah I was using sql counter on some modin tests to see the impact. For example, test_describe_numeric_only will reduce 6 describe query with this change. I also tried to run workload tests in https://github.com/snowflakedb/snowpark-pandas-internal, and I can use my own branch to check the number of describe queries.

Copy link
Contributor

@sfc-gh-helmeleegy sfc-gh-helmeleegy 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!

@sfc-gh-joshi sfc-gh-joshi changed the title SNOW-1748403: Use Aggregare.aggregate_expressions to infer quoted identifiers SNOW-1748403: Use Aggregate.aggregate_expressions to infer quoted identifiers Oct 29, 2024
@sfc-gh-jdu sfc-gh-jdu merged commit d3b23eb into main Oct 31, 2024
43 checks passed
@sfc-gh-jdu sfc-gh-jdu deleted the jdu-SNOW-1748403-agg-expr branch October 31, 2024 17:24
@github-actions github-actions bot locked and limited conversation to collaborators Oct 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants