Removes special null-but-not-null case in multiple comparison columns#11035
Removes special null-but-not-null case in multiple comparison columns#11035egalpin wants to merge 2 commits intoapache:masterfrom
Conversation
| "Upsert comparison column: %s must be comparable", columnName); | ||
| comparisonValues[i] = (Comparable) comparisonValue; | ||
| } | ||
|
|
There was a problem hiding this 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.
| if (comparisonResult != 0) { | ||
| return comparisonResult; | ||
| } | ||
| int comparisonResult = comparisonValue.compareTo(otherComparisonValue); |
There was a problem hiding this comment.
no more null-checking required, because _values will always hold all non-null entries (where some may be <defaultNullValue>
| } else { | ||
| comparisonResult = comparisonValue.compareTo(otherComparisonValue); | ||
| } | ||
| int comparisonResult = comparisonValue.compareTo(otherComparisonValue); |
There was a problem hiding this comment.
no more null-checking required, because _values will always hold all non-null entries (where some may be <defaultNullValue>
Codecov Report
@@ Coverage Diff @@
## master #11035 +/- ##
==========================================
- Coverage 0.11% 0.00% -0.12%
==========================================
Files 2199 2184 -15
Lines 118779 118378 -401
Branches 18000 17940 -60
==========================================
- Hits 137 0 -137
+ Misses 118622 118378 -244
+ Partials 20 0 -20
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 22 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
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 #11027 |
|
Sounds good! Let me test it out with restart flows as well. |
egalpin
force-pushed
the
egalpin/remove-special-null-case-multi-comp-col
branch
from
1e05dd6 to
ff178f4
Compare
|
Picked the solution in #11044 |
4 participants
Relates to #11027 (and #10704).
The summary of changes in this PR is: don't have
nullinComparisonColumnslist ofComparables, ever.I believe this PR represents a way to avoid getting into a situation where #11027 would be needed to recover.
Given a column for which
isNull(<docId>)returns true, previously an explicitnullwould be stored in the list of Comparables. After this PR, thedefaultValuefor that column would be stored in the list of Comparables instead ofnull. This allows for "regular" use ofremoveNullValueFieldfor all (post-combined) non-null comparison columns.cc @KKcorps @Jackie-Jiang thoughts?