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

Move intervalToNanos out of the Calcite files and into our parser utils functions #120

Merged
merged 3 commits into from
Jan 6, 2025

Conversation

njriasan
Copy link
Contributor

@njriasan njriasan commented Jan 6, 2025

Changes included in this PR

Changes to SqlParserUtil.java should only be those breaking changes that we need to back port into importing Calcite (and should only be done when we do not actively include all uses of the file already in our source code). Since intervalToNanos, which is meant to replace the intervalToMilli in Calcite requires a new function signature, every usage must exist inside our source code, so it should existing our extension files.

Ideally in the future we could also remove SqlParserUtil in general, but right now we need the remaining changes to ensure we get some default precisions because of its reuse in the validator.

Testing strategy

User facing changes

Checklist

  • Pipelines passed before requesting review. To run CI you must include [run CI] in your commit message.
  • I am familiar with the Contributing Guide
  • I have installed + ran pre-commit hooks.

@njriasan njriasan changed the base branch from main to nick/simplify_parser_definition January 6, 2025 17:07
Base automatically changed from nick/simplify_parser_definition to main January 6, 2025 18:10
Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@3e35877). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #120   +/-   ##
=======================================
  Coverage        ?   77.84%           
=======================================
  Files           ?      160           
  Lines           ?    62064           
  Branches        ?     8769           
=======================================
  Hits            ?    48312           
  Misses          ?    11626           
  Partials        ?     2126           

Copy link
Contributor

@scott-routledge2 scott-routledge2 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 @njriasan

@njriasan njriasan merged commit 49f94c8 into main Jan 6, 2025
19 of 21 checks passed
@njriasan njriasan deleted the nick/remove_unnecessary_parser_file branch January 6, 2025 23:21
DrTodd13 pushed a commit that referenced this pull request Jan 21, 2025
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