Skip to content

Always handle null time values #8310

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.

Sign up for GitHub

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

Jump to bottom
Merged
Jackie-Jiang merged 1 commit into from
Mar 9, 2022
Merged

Always handle null time values #8310

Jackie-Jiang merged 1 commit into from
Mar 9, 2022

Conversation

@Jackie-Jiang
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang commented Mar 8, 2022

In #7269 we added support for null time handling, but with 2 limitations:

  • It is not enabled by default
  • It does not support custom default value

This PR addressed these 2 limitations by checking if the default value complies with the time format and is in valid time range (1971-01-01 to 2071-01-01). If so, use the custom default value; if not, fall back to the current time.

Release Notes

Removed config allowNullTimeValue from the segmentsConfig, and null time values are always filled with default values

@Jackie-Jiang Jackie-Jiang added feature release-notes Referenced by PRs that need attention when compiling the next release notes labels Mar 8, 2022
@Jackie-Jiang Jackie-Jiang mentioned this pull request Mar 8, 2022
Closed
@Jackie-Jiang Jackie-Jiang force-pushed the always_allow_null_time branch 2 times, most recently from 06ce145 to d9da272 Compare March 8, 2022 18:02
@codecov-commenter
Copy link

codecov-commenter commented Mar 9, 2022

Codecov Report

Merging #8310 (d9da272) into master (a208954) will decrease coverage by 35.06%.
The diff coverage is 0.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #8310       +/-   ##
=============================================
- Coverage     64.06%   29.00%   -35.07%     
=============================================
  Files          1590     1623       +33     
  Lines         83774    85303     +1529     
  Branches      12694    12858      +164     
=============================================
- Hits          53668    24739    -28929     
- Misses        26244    58305    +32061     
+ Partials       3862     2259     -1603
Flag Coverage Δ
integration1 29.00% <0.00%> (?)
unittests1 ?
unittests2 ?

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

Impacted Files Coverage Δ
.../local/recordtransformer/NullValueTransformer.java 0.00% <0.00%> (-90.33%) ⬇️
...he/pinot/segment/local/utils/TableConfigUtils.java 0.00% <ø> (-66.07%) ⬇️
...ig/table/SegmentsValidationAndRetentionConfig.java 0.00% <ø> (-96.08%) ⬇️
...rc/main/java/org/apache/pinot/spi/data/Schema.java 0.00% <ø> (-72.79%) ⬇️
...he/pinot/spi/utils/builder/TableConfigBuilder.java 0.00% <ø> (-82.88%) ⬇️
.../java/org/apache/pinot/spi/utils/BooleanUtils.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/config/table/FSTType.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/data/MetricFieldSpec.java 0.00% <0.00%> (-100.00%) ⬇️
...va/org/apache/pinot/spi/utils/BigDecimalUtils.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/common/tier/TierFactory.java 0.00% <0.00%> (-100.00%) ⬇️
... and 1336 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a208954...d9da272. Read the comment docs.

@Jackie-Jiang Jackie-Jiang mentioned this pull request Mar 9, 2022
Closed
@Jackie-Jiang Jackie-Jiang force-pushed the always_allow_null_time branch 2 times, most recently from bfc8ddb to 9e58843 Compare March 9, 2022 04:03
@Jackie-Jiang Jackie-Jiang force-pushed the always_allow_null_time branch from 9e58843 to 351b72c Compare March 9, 2022 06:32
@Jackie-Jiang Jackie-Jiang merged commit 53fa9a5 into apache:master Mar 9, 2022
@Jackie-Jiang Jackie-Jiang deleted the always_allow_null_time branch March 9, 2022 17:22
@Jackie-Jiang Jackie-Jiang mentioned this pull request Mar 14, 2022
Merged
@mcvsubbu
Copy link
Contributor

mcvsubbu commented Apr 21, 2022

@Jackie-Jiang this should be labelled backward-incompat if anyone had the setting for allowNullTimeValue ?

@Jackie-Jiang
Copy link
Contributor Author

Jackie-Jiang commented Apr 21, 2022

@mcvsubbu It should have the same behavior as setting allowNullTimeValue before (always enabled). The only difference is that when users explicitly put the default value, we will fill the default value instead of the current time, which is the expected behavior. Do you run into issues regarding this feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mayankshriv mayankshriv mayankshriv approved these changes

@xiangfu0 xiangfu0 Awaiting requested review from xiangfu0

@chenboat chenboat Awaiting requested review from chenboat

Assignees

No one assigned

Labels

feature release-notes Referenced by PRs that need attention when compiling the next release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Jackie-Jiang @codecov-commenter @mcvsubbu @mayankshriv