You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/09/09 11:34:39 UTC
[GitHub] [pinot] KKcorps opened a new pull request, #9355: [WIP] Add sample value in datetime fieldspec to check correct format
KKcorps opened a new pull request, #9355:
URL: https://github.com/apache/pinot/pull/9355
Add support for new `sampleValue` field to check the correct timestamp format during schema creation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9355: Handle Invalid timestamps
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9355:
URL: https://github.com/apache/pinot/pull/9355#discussion_r974727851
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/SchemaUtils.java:
##########
@@ -198,6 +199,17 @@ private static void validateDateTimeFieldSpec(DateTimeFieldSpec dateTimeFieldSpe
}
}
+ Object sampleValue = dateTimeFieldSpec.getSampleValue();
+ if (sampleValue != null) {
+ long sampleTimestampValue = (sampleValue instanceof String) ? formatSpec.fromFormatToMillis((String) sampleValue)
+ : formatSpec.fromFormatToMillis(String.valueOf(sampleValue));
Review Comment:
```suggestion
long sampleTimestampValue;
try {
sampleTimestampValue = formatSpec.fromFormatToMillis(sampleValue.toString());
} catch (Exception e) {
throw new IllegalArgumentException(...);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] KKcorps merged pull request #9355: Handle Invalid timestamps
Posted by GitBox <gi...@apache.org>.
KKcorps merged PR #9355:
URL: https://github.com/apache/pinot/pull/9355
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #9355: [WIP] Add sample value in datetime fieldspec to check correct format
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9355:
URL: https://github.com/apache/pinot/pull/9355#issuecomment-1241920898
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9355?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#9355](https://codecov.io/gh/apache/pinot/pull/9355?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a79884e) into [master](https://codecov.io/gh/apache/pinot/commit/33dc520f3a00b73a07c1fd1d8c7e8d26cc7e5de9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (33dc520) will **decrease** coverage by `34.98%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #9355 +/- ##
=============================================
- Coverage 63.38% 28.40% -34.99%
+ Complexity 4755 53 -4702
=============================================
Files 1831 1872 +41
Lines 97999 99933 +1934
Branches 14990 15215 +225
=============================================
- Hits 62115 28382 -33733
- Misses 31299 68779 +37480
+ Partials 4585 2772 -1813
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `26.09% <0.00%> (?)` | |
| integration2 | `24.85% <0.00%> (?)` | |
| unittests1 | `?` | |
| unittests2 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9355?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...a/org/apache/pinot/spi/data/DateTimeFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/9355/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EYXRlVGltZUZpZWxkU3BlYy5qYXZh) | `0.00% <0.00%> (-95.56%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/9355/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/9355/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/9355/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/9355/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/9355/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/user/RoleType.java](https://codecov.io/gh/apache/pinot/pull/9355/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3VzZXIvUm9sZVR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/9355/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/utils/NullValueUtils.java](https://codecov.io/gh/apache/pinot/pull/9355/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvTnVsbFZhbHVlVXRpbHMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/pinot/pull/9355/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1533 more](https://codecov.io/gh/apache/pinot/pull/9355/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #9355: Handle Invalid timestamps
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #9355:
URL: https://github.com/apache/pinot/pull/9355#issuecomment-1251622246
Let's also update the user doc about the new field
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9355: Handle Invalid timestamps
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9355:
URL: https://github.com/apache/pinot/pull/9355#discussion_r967431208
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFieldSpec.java:
##########
@@ -119,6 +137,15 @@ public DateTimeFormatSpec getFormatSpec() {
formatSpec = new DateTimeFormatSpec(_format);
_formatSpec = formatSpec;
}
+
+ if (_sampleValue != null) {
Review Comment:
We might not want to validate the sample value here. Suggest adding a method `validateSampleValue()` and call it when uploading the schema.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFieldSpec.java:
##########
@@ -70,13 +72,19 @@ public DateTimeFieldSpec() {
* 2) if a time column is defined in hoursSinceEpoch (format=1:HOURS:EPOCH), and the data buckets are 1 hours,
* the granularity will be 1:HOURS
*/
- public DateTimeFieldSpec(String name, DataType dataType, String format, String granularity) {
+ public DateTimeFieldSpec(String name, DataType dataType, String format, String granularity,
+ @Nullable String sampleValue) {
super(name, dataType, true);
_format = format;
_granularity = granularity;
_formatSpec = new DateTimeFormatSpec(format);
_granularitySpec = new DateTimeGranularitySpec(granularity);
+ _sampleValue = sampleValue;
Review Comment:
Sample value should be an `Object` (e.g. user may put long number as the sample value)
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -765,6 +766,24 @@ private void writeMetadata()
"Invalid segment start/end time: %s (in millis: %s/%s) for time column: %s, must be between: %s",
timeInterval, timeInterval.getStartMillis(), timeInterval.getEndMillis(), timeColumnName,
TimeUtils.VALID_TIME_INTERVAL);
+ } else {
+ long now = System.currentTimeMillis();
Review Comment:
We don't want to change the start/end time within the segment metadata, instead we want to enhance the time management code to gracefully handle the segments with invalid time values.
Suggest using a separate PR for the time management change, and only keep the sample value check in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org