Skip to content

[CALCITE-6311] Support PostgreSQL DATE_PART#3794

Merged
mihaibudiu merged 1 commit intoapache:mainfrom
Bit-Quill:calcite-6311
Jun 7, 2024
Merged

[CALCITE-6311] Support PostgreSQL DATE_PART#3794
mihaibudiu merged 1 commit intoapache:mainfrom
Bit-Quill:calcite-6311

Conversation

@normanj-bitquill
Copy link
Copy Markdown
Contributor

  • PostgreSQL expects the first argument to be a string
  • The first argument can be any expression that results in a valid time unit string
  • RedShift and PostgreSQL share the same implementation

@normanj-bitquill
Copy link
Copy Markdown
Contributor Author

Since RedShift and PostgreSQL share the same implementation, they both now support strings or raw time units as the first argument.

If a string is used for the first argument, DAYOFWEEK, DAYOFYEAR and ISODOY are not working due to limitations in the TimeFrames class.
https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/rel/type/TimeFrames.java#L86

Validation of the string argument is done against the map generated in TimeFrames.

// May need to convert the first argument from a String to a TimeUnitRange
final Object timeUnitRangeObj = translator.getLiteralValue(argValueList.get(0));
final TimeUnitRange timeUnitRange;
if (timeUnitRangeObj instanceof String) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what happens if this conversion fails?
Can you have a test for that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

During validation, it will test the value and return an error if not valid.
https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java#L3484

I have added a test for this error. It results in an error message like this:

java.sql.SQLException: Error while executing SQL "select date_part('foo', timestamp '2022-06-03 12:15:48.678')": From line 1, column 18 to line 1, column 22: 'foo' is not a valid time frame

case "MILLENNIUM":
return TimeUnit.MILLENNIUM;
default:
throw new IllegalArgumentException();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe a more verbose error message could be useful.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have added a message to the exception.

@mihaibudiu
Copy link
Copy Markdown
Contributor

you have a line which exceeds 100 chars.
you can squash the commits for merging

@mihaibudiu mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jun 4, 2024
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 4, 2024

* PostgreSQL expects the first argument to be a string
* The first argument can be any expression that results in a valid time unit string
* RedShift and PostgreSQL share the same implementation
@normanj-bitquill
Copy link
Copy Markdown
Contributor Author

@mihaibudiu The commits have been squashed. The CI failure is from running out of heap space in the sonar task.

@mihaibudiu mihaibudiu merged commit d7a0f99 into apache:main Jun 7, 2024
@normanj-bitquill normanj-bitquill deleted the calcite-6311 branch August 27, 2024 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants