[
https://issues.apache.org/jira/browse/IMPALA-15280?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18118983#comment-18118983
]
ASF subversion and git services commented on IMPALA-15280:
----------------------------------------------------------
Commit 1f2018be7bbb060cae71f18f4d6c42c527019d98 in impala's branch
refs/heads/master from Aleksandr Efimov
[ https://gitbox.apache.org/repos/asf?p=impala.git;h=1f2018be7 ]
IMPALA-15280: Take referenced partitions from the final plan
HdfsScanNode registers its partitions with the descriptor table from
computeScanRangeLocations(), which runs while the scan's subtree is
being built. The descriptor table therefore describes every scan the
planner ever constructed, not the scans that ended up in the plan. A
subtree that is replaced afterwards leaves its partitions behind, and
toThrift() ships partition descriptors that nothing reads. The TODO
above the tuple descriptors in DescriptorTable.toThrift() already names
this situation.
Have each HdfsScanNode remember the ids it registered, and once the
plan is final, keep the union over the scans still reachable from the
plan fragments. Taking what the surviving scans registered, rather than
deriving the partitions again, is what makes this safe: HdfsScanNode is
the only caller of addReferencedPartition(), so the new set is by
construction a subset of the old one, it is the same set whenever every
scan that was built survives, and no scan can lose a partition it still
reads.
Deriving the partitions again from the scan would not give the same
answer. The loop in computeScanRangeLocations() walks
getSampledOrRawPartitions(), so a TABLESAMPLE scan registers the
sampled partitions and not partitions_, and a scan under a simple limit
leaves the loop once it has rows enough and never reaches the rest.
What each scan registered is the only record of what it asked for.
The walk covers every fragment, not the getFragmentsInPlanPreorder()
subset that createPlanExecInfo() uses: that one skips fragments reached
through a join build, and a scan below one of those still runs.
A table whose scans are all gone now ends up with an empty set rather
than the partitions its replaced scan had registered. That state is not
new: getReferencedPartitions() allocates an empty set for any table
toThrift() asks about, so a table whose tuple is still materialized
while nothing scans it already ships a descriptor with no partitions -
the situation the TODO above the tuple descriptors names. The backend
takes it today.
The EmptySetNode replacements on master - createScanNode() on conjuncts
implied false, the empty SPJ result set, dropped union operands - all
happen before the discarded side is planned, so no plan is expected to
change here. This is a prerequisite for IMPALA-7996, which replaces the
pruned input of a constant-false outer join after that input has been
built and trips PlannerTestBase.testHdfsPartitionsReferenced without
it.
Testing:
- PlannerTest, which asserts on every query that each partition in the
descriptor table is covered by a scan range. PlannerTestBase used to
check only the single-node plan and now checks the distributed and
parallel plans too.
Change-Id: I553281e7c77e3297585360f37160bc3f9d69a208
Assisted-by: claude-opus-5 (Claude Code)
Reviewed-on: http://gerrit.cloudera.org:8080/24723
Reviewed-by: Impala Public Jenkins <[email protected]>
Tested-by: Impala Public Jenkins <[email protected]>
> Descriptor table lists partitions of scans that are not in the final plan
> -------------------------------------------------------------------------
>
> Key: IMPALA-15280
> URL: https://issues.apache.org/jira/browse/IMPALA-15280
> Project: IMPALA
> Issue Type: Improvement
> Components: Frontend
> Reporter: Aleksandr Efimov
> Assignee: Aleksandr Efimov
> Priority: Major
>
> HdfsScanNode registers the partitions it will read with the descriptor table
> from computeScanRangeLocations(), which init() calls while the scan's subtree
> is being built:
>
> {code:java}
> analyzer.getDescTbl().addReferencedPartition(tbl_, partition.getId());
> {code}
>
> DescriptorTable.toThrift() then sends the backend exactly those partitions
> for every table except a table sink's target, which keeps all of them. So the
> set describes every scan the planner ever constructed rather than the scans
> that ended up in the plan: anything built and later replaced leaves its
> partitions behind, and the backend gets partition metadata that no scan
> reads. The TODO above the tuple descriptors in toThrift() already names the
> situation:
>
> {code:java}
> // TODO: Ideally, we should call tupleDesc.checkIsExecutable() here, but there
> // currently are several situations in which we send materialized tuples
> without
> // a mem layout to the BE, e.g., when unnesting unions or when replacing plan
> // trees with an EmptySetNode.
> {code}
>
> I did not find a query on master that reproduces this. The subtree
> replacements I looked at - createScanNode() when the conjuncts are implied
> false, the empty SPJ result set in createSelectPlan(), and union operands
> dropped for the same reason - all decide before the discarded side is
> planned, so nothing is registered and then thrown away. I did not try to
> establish that no other path exists.
>
> IMPALA-7996 does hit it. It replaces the pruned input of a constant-false
> outer join after that input has been built, and
> PlannerTestBase.testHdfsPartitionsReferenced, which asserts that every
> partition in the descriptor table is covered by a scan range, fails on the
> PlannerTest#testEmpty queries it adds.
>
> Proposal: have each HdfsScanNode remember the ids it registered, and once the
> plan is final - after Planner.createPlans() in Frontend.createExecRequest(),
> before the descriptor table is serialized - keep the union over the scans
> still reachable from the plan fragments.
>
> Keeping what the surviving scans registered, rather than deriving the
> partitions again, is what bounds the risk. HdfsScanNode is the only caller of
> addReferencedPartition(), so the resulting set is by construction a subset of
> today's, and it is the same set whenever every scan that was built survives -
> which, per the paragraph above, is every plan on master. No plan should
> change here; the change is a prerequisite rather than a fix with its own
> visible effect.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
