sql (fix): Support special date and time functions #3921
Conversation
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
There was a problem hiding this comment.
Hello @takezoe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request to help everyone get up to speed quickly.
The intent of this PR, based on the title, is to add support for parsing and representing standard SQL special date and time functions within the airframe-sql parser. This includes functions like CURRENT_DATE, CURRENT_TIME, CURRENT_TIMESTAMP, LOCALTIME, and LOCALTIMESTAMP, along with their optional precision arguments.
Highlights
- Support for SQL Date/Time Functions: This pull request introduces the ability to parse and represent several standard SQL date and time functions:
CURRENT_DATE,CURRENT_TIME,CURRENT_TIMESTAMP,LOCALTIME, andLOCALTIMESTAMP. - Parsing Logic Implementation: The core parsing logic for these functions has been added to the
SQLInterpreter, specifically within a newvisitSpecialDateTimeFunctionmethod, which maps the parsed tokens to the corresponding model expressions. - Model Representation Update: The
Expressionmodel has been updated to correctly represent these functions, including adding ansqlExprmethod to theCurrentTimeBaseabstract class to handle the string representation with optional precision. - Added Test Coverage: New tests have been added in
SQLExprTestto verify that the parser correctly handles these special date and time functions, both with and without precision arguments.
Changelog
- airframe-sql/src/main/scala/wvlet/airframe/sql/model/Expression.scala
- Added
sqlExprmethod toCurrentTimeBaseabstract class to generate SQL string representation including optional precision.
- Added
- airframe-sql/src/main/scala/wvlet/airframe/sql/parser/SQLInterpreter.scala
- Added
visitSpecialDateTimeFunctionmethod to parseCURRENT_DATE,CURRENT_TIME,CURRENT_TIMESTAMP,LOCALTIME, andLOCALTIMESTAMPfrom the ANTLR context. - Mapped parsed date/time function tokens to corresponding
Expressionmodel classes (CurrentDate,CurrentTime, etc.). - Extracted optional precision argument for time and timestamp functions.
- Added
- airframe-sql/src/test/scala/wvlet/airframe/sql/model/SQLExprTest.scala
- Added a new test case
special date time functionsto verify parsing of the newly supported date/time functions. - Included test cases for functions with and without precision arguments.
- Added a new test case
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Time functions arrive,
Parsing dates, precision too,
SQL understands.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
✅ I finished the code review, and didn't find any security or code quality issues. |
7c25590 to
a9c1c00
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces support for special date and time functions like CURRENT_DATE, CURRENT_TIME, CURRENT_TIMESTAMP, LOCALTIME, and LOCALTIMESTAMP, including handling for optional precision. The changes are well-structured across the expression model, SQL interpreter, and include a good set of tests.
The implementation in Expression.scala for CurrentTimeBase.sqlExpr is clear and correctly handles precision. The tests in SQLExprTest.scala are comprehensive and cover various cases with and without precision, ensuring the sqlExpr generation is accurate.
One area for potential improvement is in SQLInterpreter.scala regarding the parsing of precision, which I've detailed in a specific comment. Overall, this is a solid contribution.
Summary of Findings
- Code Duplication in Precision Parsing: In
SQLInterpreter.scala, the logic for extracting and parsing the optional precision for date/time functions is repeated across multiple cases. Refactoring this into a local helper function would improve code clarity and maintainability, adhering to the DRY principle. This was commented on withmediumseverity.
Merge Readiness
The pull request is generally in good shape and adds valuable functionality. There is one suggestion for improving maintainability in the SQLInterpreter.scala file.
As an AI assistant, I am not authorized to approve pull requests. However, based on this review, I recommend addressing the suggested improvement. Please ensure further review and approval by authorized team members before merging.
| ctx.name.getType match { | ||
| case SqlBaseParser.CURRENT_DATE => CurrentDate(None, getLocation(ctx)) | ||
| case SqlBaseParser.CURRENT_TIME => CurrentTime(Option(ctx.precision).map(_.getText.toInt), getLocation(ctx)) | ||
| case SqlBaseParser.CURRENT_TIMESTAMP => | ||
| CurrentTimestamp(Option(ctx.precision).map(_.getText.toInt), getLocation(ctx)) | ||
| case SqlBaseParser.LOCALTIME => CurrentLocalTime(Option(ctx.precision).map(_.getText.toInt), getLocation(ctx)) | ||
| case SqlBaseParser.LOCALTIMESTAMP => | ||
| CurrentLocalTimeStamp(Option(ctx.precision).map(_.getText.toInt), getLocation(ctx)) | ||
| } |
There was a problem hiding this comment.
The logic for parsing precision (Option(ctx.precision).map(_.getText.toInt)) and getting location (getLocation(ctx)) is repeated for several cases within this match statement. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider extracting this logic into a local helper function or value.
This would make the code cleaner, especially if more date/time functions with similar patterns are added in the future. How about refactoring this to use a local helper for precision and pre-calculating the location? 1
val location = getLocation(ctx)
// Helper to parse precision from a terminal node
def parsePrecision(node: org.antlr.v4.runtime.tree.TerminalNode): Option[Int] =
Option(node).map(_.getText.toInt)
ctx.name.getType match {
case SqlBaseParser.CURRENT_DATE => CurrentDate(None, location)
case SqlBaseParser.CURRENT_TIME => CurrentTime(parsePrecision(ctx.precision), location)
case SqlBaseParser.CURRENT_TIMESTAMP => CurrentTimestamp(parsePrecision(ctx.precision), location)
case SqlBaseParser.LOCALTIME => CurrentLocalTime(parsePrecision(ctx.precision), location)
case SqlBaseParser.LOCALTIMESTAMP => CurrentLocalTimeStamp(parsePrecision(ctx.precision), location)
}Style Guide References
Footnotes
-
Following common Scala best practices, particularly the DRY (Don't Repeat Yourself) principle, helps in creating more maintainable and readable code. Repetitive code blocks can be refactored into helper methods or values to reduce redundancy and centralize logic. ↩
8b406fd to
931bc97
Compare
| @@ -463,6 +463,8 @@ object SQLGenerator extends LogSupport { | |||
| s"(${values.map(printExpression).mkString(", ")})" | |||
| case Parameter(index, _) => | |||
| "?" | |||
| case e: Expression => | |||
| e.sqlExpr | |||
There was a problem hiding this comment.
It might be better to cover concrete expression types one by one rather than covering all types here to notice unintentional SQL generation?
If we would apply this, other branch will be unnecessary.
There was a problem hiding this comment.
I would cover only CurrentTimeBase type for now. Changing to cover all expression types would be another topic.
931bc97 to
ba4b29c
Compare
No description provided.