You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "egalpin (via GitHub)" <gi...@apache.org> on 2023/07/05 22:54:51 UTC
[GitHub] [pinot] egalpin opened a new pull request, #11035: Removes special null-but-not-null case in multiple comparison columns
egalpin opened a new pull request, #11035:
URL: https://github.com/apache/pinot/pull/11035
Relates to #11027 (and https://github.com/apache/pinot/pull/10704).
**The summary of changes in this PR is: don't have `null` in `ComparisonColumns` list of `Comparables`, ever.**
I believe this PR represents a way to _avoid_ getting into a situation where #11027 would be needed to recover; it doesn't, however, provide a way to recover from a "bad state" that #11027 _does_ help recover from.
Given a column for which `isNull(<docId>)` returns true, previously an explicit `null` would be stored in the list of Comparables. After this PR, the `defaultValue` for that column would be stored in the list of Comparables instead of `null`. This allows for "regular" use of `removeNullValueField` for all (post-combined) non-null comparison columns.
cc @KKcorps @Jackie-Jiang thoughts?
--
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] egalpin commented on a diff in pull request #11035: Removes special null-but-not-null case in multiple comparison columns
Posted by "egalpin (via GitHub)" <gi...@apache.org>.
egalpin commented on code in PR #11035:
URL: https://github.com/apache/pinot/pull/11035#discussion_r1253734490
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -550,12 +550,12 @@ private Comparable getComparisonValue(GenericRow row) {
"Documents must have exactly 1 non-null comparison column value");
comparableIndex = i;
-
- Object comparisonValue = row.getValue(columnName);
- Preconditions.checkState(comparisonValue instanceof Comparable,
- "Upsert comparison column: %s must be comparable", columnName);
- comparisonValues[i] = (Comparable) comparisonValue;
}
+
Review Comment:
this change means that we always set a non-null value for each index of `comparisonValues`, whereas previous all indices != `comparableIndex` would be null.
--
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] egalpin commented on a diff in pull request #11035: Removes special null-but-not-null case in multiple comparison columns
Posted by "egalpin (via GitHub)" <gi...@apache.org>.
egalpin commented on code in PR #11035:
URL: https://github.com/apache/pinot/pull/11035#discussion_r1253735749
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ComparisonColumns.java:
##########
@@ -78,18 +69,10 @@ public int compareTo(ComparisonColumns other) {
// _comparisonColumns should only at most one non-null comparison value for newly ingested data. If not, it is
// the user's responsibility. There is no attempt to guarantee behavior in the case where there are multiple
// non-null values
- int comparisonResult;
-
Comparable comparisonValue = _values[_comparableIndex];
Comparable otherComparisonValue = other.getValues()[_comparableIndex];
- if (otherComparisonValue == null) {
- // Keep this record because the existing record has no value for the same comparison column, therefore the
- // (lack of) existing value could not possibly cause the new value to be rejected.
- comparisonResult = 1;
- } else {
- comparisonResult = comparisonValue.compareTo(otherComparisonValue);
- }
+ int comparisonResult = comparisonValue.compareTo(otherComparisonValue);
Review Comment:
no more null-checking required, because _values will always hold all non-null entries (where some may be `<defaultNullValue>`
--
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 commented on pull request #11035: Removes special null-but-not-null case in multiple comparison columns
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on PR #11035:
URL: https://github.com/apache/pinot/pull/11035#issuecomment-1623497277
Sounds good! Let me test it out with restart flows as well.
--
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] egalpin commented on a diff in pull request #11035: Removes special null-but-not-null case in multiple comparison columns
Posted by "egalpin (via GitHub)" <gi...@apache.org>.
egalpin commented on code in PR #11035:
URL: https://github.com/apache/pinot/pull/11035#discussion_r1253735661
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ComparisonColumns.java:
##########
@@ -52,18 +52,9 @@ public int compareToSealed(ComparisonColumns other) {
continue;
}
- // Always keep the record with non-null value, or that with the greater comparisonResult
- if (comparisonValue == null) {
- // implies comparisonValue == null && otherComparisonValue != null
- return -1;
- } else if (otherComparisonValue == null) {
- // implies comparisonValue != null && otherComparisonValue == null
- return 1;
- } else {
- int comparisonResult = comparisonValue.compareTo(otherComparisonValue);
- if (comparisonResult != 0) {
- return comparisonResult;
- }
+ int comparisonResult = comparisonValue.compareTo(otherComparisonValue);
Review Comment:
no more null-checking required, because _values will always hold all non-null entries (where some may be `<defaultNullValue>`
--
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 closed pull request #11035: Removes special null-but-not-null case in multiple comparison columns
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang closed pull request #11035: Removes special null-but-not-null case in multiple comparison columns
URL: https://github.com/apache/pinot/pull/11035
--
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 #11035: Removes special null-but-not-null case in multiple comparison columns
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11035:
URL: https://github.com/apache/pinot/pull/11035#issuecomment-1624253163
Picked the solution in #11044
--
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] egalpin commented on pull request #11035: Removes special null-but-not-null case in multiple comparison columns
Posted by "egalpin (via GitHub)" <gi...@apache.org>.
egalpin commented on PR #11035:
URL: https://github.com/apache/pinot/pull/11035#issuecomment-1622693562
Confirmed with local integration (in addition to unit tests) that this change results in all the same desired outcomes as before, but lacks the undesirable outcome noted in https://github.com/apache/pinot/pull/11027
--
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 #11035: Removes special null-but-not-null case in multiple comparison columns
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11035:
URL: https://github.com/apache/pinot/pull/11035#issuecomment-1622691204
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11035?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11035](https://app.codecov.io/gh/apache/pinot/pull/11035?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (389bf94) into [master](https://app.codecov.io/gh/apache/pinot/commit/2fabbe40057d99afa497071f7083b4f16e40137a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (2fabbe4) will **increase** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11035 +/- ##
=======================================
Coverage 0.11% 0.11%
=======================================
Files 2199 2199
Lines 118779 118770 -9
Branches 18000 17995 -5
=======================================
Hits 137 137
+ Misses 118622 118613 -9
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `?` | |
| integration1temurin17 | `0.00% <0.00%> (ø)` | |
| integration1temurin20 | `0.00% <0.00%> (ø)` | |
| unittests1temurin17 | `0.00% <0.00%> (ø)` | |
| unittests1temurin20 | `0.00% <0.00%> (ø)` | |
| unittests2temurin11 | `0.11% <0.00%> (ø)` | |
| unittests2temurin17 | `0.11% <0.00%> (ø)` | |
| unittests2temurin20 | `0.11% <0.00%> (ø)` | |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/11035?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://app.codecov.io/gh/apache/pinot/pull/11035?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [.../pinot/segment/local/upsert/ComparisonColumns.java](https://app.codecov.io/gh/apache/pinot/pull/11035?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvQ29tcGFyaXNvbkNvbHVtbnMuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...not/segment/local/upsert/PartialUpsertHandler.java](https://app.codecov.io/gh/apache/pinot/pull/11035?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvUGFydGlhbFVwc2VydEhhbmRsZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...apache/pinot/segment/local/upsert/UpsertUtils.java](https://app.codecov.io/gh/apache/pinot/pull/11035?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvVXBzZXJ0VXRpbHMuamF2YQ==) | `0.00% <0.00%> (ø)` | |
: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=apache)
--
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