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

Remove Calcite return type override for FROM_DATE_TIME scalar function in PinotOperatorTable #14075

Closed

Conversation

yashmayya
Copy link
Collaborator

image

  • The FROM_DATE_TIME function only has a scalar implementation in Pinot (and no transform function equivalent) that returns a long, so it isn't too clear why the Calcite return type was forced to TIMESTAMP.

@yashmayya yashmayya added the multi-stage Related to the multi-stage query engine label Sep 25, 2024
@yashmayya yashmayya force-pushed the fromdatetime-return-type-fix branch from 9410ad9 to 4713ca7 Compare September 25, 2024 08:43
@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.04%. Comparing base (59551e4) to head (4713ca7).
Report is 1086 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #14075      +/-   ##
============================================
+ Coverage     61.75%   64.04%   +2.29%     
- Complexity      207     1537    +1330     
============================================
  Files          2436     2596     +160     
  Lines        133233   143292   +10059     
  Branches      20636    21948    +1312     
============================================
+ Hits          82274    91770    +9496     
+ Misses        44911    44777     -134     
- Partials       6048     6745     +697     
Flag Coverage Δ
custom-integration1 100.00% <ø> (+99.99%) ⬆️
integration 100.00% <ø> (+99.99%) ⬆️
integration1 100.00% <ø> (+99.99%) ⬆️
integration2 0.00% <ø> (ø)
java-11 63.95% <ø> (+2.24%) ⬆️
java-21 63.93% <ø> (+2.31%) ⬆️
skip-bytebuffers-false 64.01% <ø> (+2.27%) ⬆️
skip-bytebuffers-true 63.86% <ø> (+36.13%) ⬆️
temurin 64.04% <ø> (+2.29%) ⬆️
unittests 64.03% <ø> (+2.29%) ⬆️
unittests1 55.60% <ø> (+8.71%) ⬆️
unittests2 34.51% <ø> (+6.78%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yashmayya yashmayya marked this pull request as ready for review September 25, 2024 09:39
@yashmayya
Copy link
Collaborator Author

@Jackie-Jiang @xiangfu0 do you know why we had overridden the return type of this scalar function from LONG to TIMESTAMP in the multistage engine?

@Jackie-Jiang
Copy link
Contributor

I think the idea is to return TIMESTAMP type when the value is a timestamp. The reason why this UDF returns LONG in v1 is that it was introduced before TIMESTAMP data type was added, and we cannot easily change the v1 behavior without breaking backward compatibility

@yashmayya yashmayya closed this Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multi-stage Related to the multi-stage query engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants