Skip to content

Conversation

@KKcorps
Copy link
Contributor

@KKcorps KKcorps commented Sep 9, 2022

  • Add support for new sampleValue field to check the correct timestamp format during schema creation.

@KKcorps KKcorps marked this pull request as ready for review September 9, 2022 11:34
@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2022

Codecov Report

Merging #9355 (7774ed5) into master (33dc520) will increase coverage by 6.33%.
The diff coverage is 94.73%.

@@             Coverage Diff              @@
##             master    #9355      +/-   ##
============================================
+ Coverage     63.38%   69.71%   +6.33%     
+ Complexity     4755     4710      -45     
============================================
  Files          1831     1890      +59     
  Lines         97999   100760    +2761     
  Branches      14990    15352     +362     
============================================
+ Hits          62115    70246    +8131     
+ Misses        31299    25523    -5776     
- Partials       4585     4991     +406     
Flag Coverage Δ
integration1 25.95% <0.00%> (?)
integration2 24.60% <0.00%> (?)
unittests1 66.99% <94.73%> (+0.06%) ⬆️
unittests2 15.34% <0.00%> (+0.06%) ⬆️

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

Impacted Files Coverage Δ
.../apache/pinot/segment/local/utils/SchemaUtils.java 96.34% <85.71%> (-1.00%) ⬇️
...a/org/apache/pinot/spi/data/DateTimeFieldSpec.java 96.36% <100.00%> (+0.80%) ⬆️
.../java/org/apache/pinot/spi/utils/BooleanUtils.java 33.33% <0.00%> (-66.67%) ⬇️
...e/pinot/core/common/datatable/DataTableImplV4.java 71.42% <0.00%> (-28.58%) ⬇️
...a/org/apache/pinot/core/plan/GlobalPlanImplV0.java 86.66% <0.00%> (-13.34%) ⬇️
.../query/runtime/plan/serde/QueryPlanSerDeUtils.java 90.16% <0.00%> (-9.84%) ⬇️
...va/org/apache/pinot/query/runtime/QueryRunner.java 78.37% <0.00%> (-9.13%) ⬇️
...pinot/query/runtime/operator/HashJoinOperator.java 80.00% <0.00%> (-8.24%) ⬇️
...ache/pinot/core/common/datablock/RowDataBlock.java 70.58% <0.00%> (-5.89%) ⬇️
...he/pinot/segment/local/segment/store/IndexKey.java 65.00% <0.00%> (-5.00%) ⬇️
... and 487 more

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

@KKcorps KKcorps changed the title [WIP] Add sample value in datetime fieldspec to check correct format Handle Invalid timestamps Sep 9, 2022
@KKcorps KKcorps force-pushed the time_validation_patch branch from 58e966f to f80344c Compare September 9, 2022 15:40
@KKcorps KKcorps force-pushed the time_validation_patch branch from f80344c to a79884e Compare September 14, 2022 14:54
@KKcorps KKcorps force-pushed the time_validation_patch branch from d56bd57 to 7774ed5 Compare September 16, 2022 10:02
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

Comment on lines 204 to 205
long sampleTimestampValue = (sampleValue instanceof String) ? formatSpec.fromFormatToMillis((String) sampleValue)
: formatSpec.fromFormatToMillis(String.valueOf(sampleValue));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
long sampleTimestampValue = (sampleValue instanceof String) ? formatSpec.fromFormatToMillis((String) sampleValue)
: formatSpec.fromFormatToMillis(String.valueOf(sampleValue));
long sampleTimestampValue;
try {
sampleTimestampValue = formatSpec.fromFormatToMillis(sampleValue.toString());
} catch (Exception e) {
throw new IllegalArgumentException(...);
}

@Jackie-Jiang Jackie-Jiang added the Configuration Config changes (addition/deletion/change in behavior) label Sep 19, 2022
@Jackie-Jiang
Copy link
Contributor

Let's also update the user doc about the new field

@KKcorps KKcorps merged commit 165e26f into apache:master Sep 23, 2022
61yao pushed a commit to 61yao/pinot that referenced this pull request Oct 3, 2022
* Add sample value in datetime fieldspec to check correct format

* Bug fix and add test cases for sample value

* Fix linting

* throw exception in case of error

Co-authored-by: Kartik Khare <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Configuration Config changes (addition/deletion/change in behavior)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants