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

Fix transformation to string for BOOLEAN and TIMESTAMP #9287

Merged
merged 1 commit into from
Aug 30, 2022

Conversation

Jackie-Jiang
Copy link
Contributor

For BOOLEAN and TIMESTAMP (logical types), when reading them as string, we should use the logical type to return true, 2022-02-02 00:00:00.000 instead of the int/long value.
With the fix in BaseTransformFunction, the CAST transform function can be greatly simplified.

longValues = transformToLongValuesSV(projectionBlock);
for (int i = 0; i < length; i++) {
_stringValuesSV[i] = new Timestamp(longValues[i]).toString();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

missing break;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't really need break for default branch. Changed it to if-else branches for clarity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, we do have linter check for that lol

@codecov-commenter
Copy link

codecov-commenter commented Aug 28, 2022

Codecov Report

Merging #9287 (6123d8d) into master (70ecda4) will decrease coverage by 43.78%.
The diff coverage is 24.22%.

❗ Current head 6123d8d differs from pull request most recent head b6270d8. Consider uploading reports for the commit b6270d8 to get more accurate results

@@              Coverage Diff              @@
##             master    #9287       +/-   ##
=============================================
- Coverage     68.60%   24.82%   -43.79%     
+ Complexity     5007       53     -4954     
=============================================
  Files          1867     1855       -12     
  Lines         99631    99217      -414     
  Branches      15150    15126       -24     
=============================================
- Hits          68351    24627    -43724     
- Misses        26362    72105    +45743     
+ Partials       4918     2485     -2433     
Flag Coverage Δ
integration1 ?
integration2 24.82% <24.22%> (?)
unittests1 ?
unittests2 ?

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

Impacted Files Coverage Δ
...a/org/apache/pinot/common/metrics/ServerMeter.java 100.00% <ø> (ø)
...core/operator/blocks/IntermediateResultsBlock.java 61.97% <ø> (-22.99%) ⬇️
...ator/transform/function/BaseTransformFunction.java 9.31% <11.66%> (-42.19%) ⬇️
...ator/transform/function/CastTransformFunction.java 30.00% <23.07%> (-40.65%) ⬇️
...java/org/apache/pinot/core/common/DataFetcher.java 54.06% <28.20%> (-35.68%) ⬇️
.../core/operator/query/SelectionOrderByOperator.java 67.71% <50.00%> (-20.13%) ⬇️
.../pinot/core/operator/InstanceResponseOperator.java 87.80% <100.00%> (+10.44%) ⬆️
...in/java/org/apache/pinot/spi/utils/BytesUtils.java 0.00% <0.00%> (-100.00%) ⬇️
.../java/org/apache/pinot/spi/utils/BooleanUtils.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/spi/trace/BaseRecording.java 0.00% <0.00%> (-100.00%) ⬇️
... and 1421 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Jackie-Jiang Jackie-Jiang force-pushed the simplify_cast branch 2 times, most recently from 221852a to e5d6462 Compare August 29, 2022 04:10
@Jackie-Jiang Jackie-Jiang merged commit 74fd91a into apache:master Aug 30, 2022
@Jackie-Jiang Jackie-Jiang deleted the simplify_cast branch August 30, 2022 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants