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

[CALCITE-2980] Implement the FORMAT clause of the CAST operator #3677

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

Anthrino
Copy link
Contributor

@Anthrino Anthrino commented Feb 8, 2024

No description provided.

@Anthrino Anthrino force-pushed the CALCITE_2980_cast_op_format_clause branch 5 times, most recently from 3070c02 to 3277903 Compare February 13, 2024 19:08
@Anthrino Anthrino force-pushed the CALCITE_2980_cast_op_format_clause branch 3 times, most recently from 9122b9f to a8e4f57 Compare February 21, 2024 00:09
@Anthrino Anthrino marked this pull request as ready for review February 21, 2024 00:44
@Anthrino Anthrino force-pushed the CALCITE_2980_cast_op_format_clause branch from a8e4f57 to 3269a70 Compare February 23, 2024 01:17
Copy link

sonarcloud bot commented Feb 23, 2024

Copy link
Contributor

@tanclary tanclary left a comment

Choose a reason for hiding this comment

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

This all looks great to me, should we mention in the commit message that this is specifically for BigQuery? Add BigQuery's FORMAT clause to CAST operator or something along those lines

@Anthrino Anthrino force-pushed the CALCITE_2980_cast_op_format_clause branch from 3269a70 to 418ade5 Compare March 13, 2024 17:29
@Anthrino Anthrino force-pushed the CALCITE_2980_cast_op_format_clause branch from 418ade5 to 2dadcd1 Compare April 4, 2024 22:04
return RexImpTable.optimize2(operand,
Expressions.call(BuiltInMethod.UNIX_TIME_TO_STRING.method,
operand));
return RexImpTable.optimize2(operand, Expressions.isConstantNull(format)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you evaluate this before the switch ? then you can just check the variable in each case

Copy link
Contributor Author

@Anthrino Anthrino Apr 5, 2024

Choose a reason for hiding this comment

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

Not exactly, we could only move the RexImpTable.optimize2() call outside the switch, but the specific method called to evaluate the CAST depends on the from and to cast data types, and with this change also if there is a FORMAT present. So the number of lines would still look the same, but we could move out one level of nesting with the optimize calls and return statements outside the switch case.
Does that seem like a better pattern?

'FMMONTH FMMonth FMmonth');
EXPR$0
AUGUST August august
!ok

select cast(date'2010-09-19' as string FORMAT
select cast(date'2010-09-19' as varchar FORMAT
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for going through these im sure that took a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to have the working tests enabled, and the disabled ones gives a status on whats yet to be supported and need to be addressed in followup JIRAs.

Copy link
Contributor

@tanclary tanclary left a comment

Choose a reason for hiding this comment

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

I think this looks good thanks for addressing all the comments, maybe we can see if @julianhyde has any suggestions otherwise merge soon?

@Anthrino Anthrino force-pushed the CALCITE_2980_cast_op_format_clause branch 3 times, most recently from c4c6a55 to c2f6806 Compare April 11, 2024 22:04
@Anthrino Anthrino force-pushed the CALCITE_2980_cast_op_format_clause branch from c2f6806 to dbf33c0 Compare April 11, 2024 22:58
Copy link

sonarcloud bot commented Apr 11, 2024

@tanclary tanclary merged commit cc1d46a into apache:main Apr 11, 2024
17 checks passed
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