kadirozde commented on PR #2428:
URL: https://github.com/apache/phoenix/pull/2428#issuecomment-6084313578
@apurtell Thanks for fixing up the merge commit and for digging into these.
I went through both commits.
3e1caf6: bounding hull instead of a full scan when widening a leading dim
This looks good to me. Widening the only remaining constrained dim to
EVERYTHING was clearly wrong, and collapsing to the [min, max] hull (the same
thing V1 does) with the result marked approximated keeps the IN list as a
residual filter, so results stay exact.
- The prefix handling in boundingHull looks right to me. Moving the lower
bound to previousKey(min) and the upper bound to nextKey(shortest prefix of
max) is correct for both the 0x00 and 0xFF separators.
- Removing the "no progress" bail-out is fine: each pass makes the trailing
dim EVERYTHING in every space, so the next trailing dim is strictly lower and
the loop always ends.
- Minor: testJoinKeyRangeWithLargeBuildSideIsBounded loads about 73k rows in
an IT. That's acceptable, but it adds noticeable runtime.
+1 on this one.
05fed39: DESC variable-length columns with prefix-related bounds
The analysis is right: for a DESC var-length column the row key stores value
|| 0xFF, so '2' sorts after '23', and every step that compares raw per-column
bytes can get the order wrong. A few concerns, though:
1. Possible extra rows when the DESC column gets its own slot. When the
compound window is shortened so the trailing DESC column is emitted separately,
that slot is now filled with dimRanges(list, d), the union of that column's
ranges across all spaces. That loses the pairing with the leading columns. For
example, (k1, k2) IN (('a','1'), ('b','10')) with k2 VARCHAR DESC becomes {a,b}
x {1,10}, which also admits ('a','10') and ('b','1'). As far as I can see,
compoundsApproximated isn't set on this path, and WhereOptimizerV2 drops the
visitor-consumed nodes from the residual filter unless r.approximated ||
keySpaceList.isApproximated(). If the RVC was consumed, those extra rows would
come back. WhereOptimizerRowKeyCoverageTest only checks that the scan admits
every matching row, so it wouldn't catch this. Could we mark the result
approximated whenever the dimRanges branch is taken (or whenever the window is
shortened), and add an exact-result test for that RVC IN case against real r
ows?
2. Always stripping the trailing separator. The previous code deliberately
kept the separator when unconstrained PK columns follow the compound, and its
comment cited QueryCompilerTest.testRVCScanBoundaries1 breaking otherwise. The
new code always strips it and relies on ScanUtil.setKey to add it back. Could
you confirm testRVCScanBoundaries1 and the other RVC scan-boundary tests still
pass with this change?
3. The prefix guards also apply to ASC and fixed-width columns.
KeySpace.unionIfMergeable and coalesceColumn have no sort-order information, so
they also block merging and coalescing for ASC var-length columns, where byte
order already matches row-key order. The prefix check in unionIfMergeable also
runs before contains(), so something like k1 LIKE '1%' OR k1 = '10' now keeps
two spaces instead of absorbing one. The cost is more spaces, which means
hitting the 65,536 bound (and the hull) sooner and producing longer skip-scan
slot lists. Would it be feasible to pass in the sort order or schema so the
guard only applies to DESC var-length columns?
4. Overlapping ranges in a slot. With prefix bounds present, coalesceColumn
now only deduplicates, so a slot can contain overlapping ranges. The comment
says SkipScanFilter navigates overlapping ranges. Since it binary-searches its
slot ranges, I'd like a targeted test (or a pointer to where that's handled)
rather than relying only on the coverage test's examples.
Summary: I'm happy with 3e1caf6 as is. For 05fed39, I think (1) needs a fix
before merging, (2) needs confirmation, and (3) and (4) are worth addressing
but are less urgent.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
