voonhous commented on code in PR #19863:
URL: https://github.com/apache/hudi/pull/19863#discussion_r3956128748
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/HudiSplitManager.java:
##########
@@ -82,6 +83,12 @@ public ConnectorSplitSource getSplits(
Constraint constraint)
{
HudiTableHandle hudiTableHandle = (HudiTableHandle) tableHandle;
+ // The planner turns a none() domain into an empty ValuesNode before
applyFilter, so a none() predicate
+ // should never reach here; guard anyway, as computePartitionKeyFilter
below throws on a none() domain.
Review Comment:
Valid, the comment only justifies the partition arm. Proposed wording:
```diff
HudiTableHandle hudiTableHandle = (HudiTableHandle) tableHandle;
// The planner turns a none() domain into an empty ValuesNode
before applyFilter, so a none() predicate
- // should never reach here; guard anyway, as
computePartitionKeyFilter below throws on a none() domain.
+ // should never reach here. Guard anyway; the two arms stop
different things. A none() partition predicate
+ // would reach computePartitionKeyFilter below, whose checkArgument
rejects it. A none() regular predicate
+ // throws nothing: it empties
TupleDomainUtils.getReferencedColumns, so every index strategy's canApply
+ // declines and nothing is pruned.
if (hudiTableHandle.getPartitionPredicates().isNone() ||
hudiTableHandle.getRegularPredicates().isNone()) {
```
Worth its own follow-up, or fold into the next change that touches this
file? The guard is unreachable either way, so I would rather not spend a
comment-only commit on it now. Happy to open one if you would rather it not
wait.
--
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]
