fix: bad downcast of ArrowResultCollector in plan mapper re-enables skipped arrow/CSR tests (#881) - #884
Merged
Conversation
…881) Root cause of the linux minimal-test failures tracked in #881: ArrowResultCollector and ResultCollector both declare PhysicalOperatorType::RESULT_COLLECTOR. In PlanMapper::getPhysicalPlan(), the plan-root marker setResultExposedToClient() was applied via an operator-type check: if (root->getOperatorType() == PhysicalOperatorType::RESULT_COLLECTOR) { root->ptrCast<ResultCollector>()->setResultExposedToClient(); } For every arrow query (resultType=ARROW), the newly created ArrowResultCollector matched the type check and was then downcast to ResultCollector, which it is not: - With RUNTIME_CHECKS (or any !NDEBUG build) dynamic_cast_checked asserts and throws, so every queryAsArrow execution failed and returned an error MaterializedQueryResult with a null iterator. Tests that call getNextArrowChunk() without checking isSuccess() then SIGSEGV in FactorizedTableIterator::hasNext (the 'Fatal signal 11' in the linux minimal-test job), and the remaining tests fail on dynamic_cast<ArrowQueryResult*>(result.get()) == nullptr and entry.relCsrResults[0] == nullptr (PROJECT_GRAPH materialization also goes through queryAsArrow). - With NDEBUG the checked cast is a reinterpret_cast and setResultExposedToClient() writes one byte at ResultCollector::internalResultTable's offset (192), which in an ArrowResultCollector is localState.batchIndex - undefined behaviour on every arrow query. Fix: mark the client-facing result table on the freshly created regular ResultCollector inside the non-arrow branch of the if/else, instead of matching on the shared operator-type enum afterwards. Also re-enable the ten arrow/CSR tests that were skipped for #881: ArrowTest.queryAsArrow, getArrowResult, queryAsArrowDirectCSRRowIDProjection (+WithFourThreads), queryAsArrowTracksCSRMetadataWithoutRelIDs / WithRelIDsAndExtraColumns / DoesNotTrackCSRMetadataForNonCSRShape, ProjectGraphCsrTest.materializesArrowCsr / materializedCsrSurvivesConsumingQueries, ReadOnlyTest.ProjectGraphOnReadOnlyDatabase. Note on the earlier investigation: the 'corrupted task clone' evidence in the issue (this == sharedState, garbage batch indices, csr metadata present for CSR-free queries) was an artefact of the temporary LBUG_ARROW_DEBUG tracing, whose fprintf macro appended the thread-id string after the format's varargs, shifting every printed value by one slot; the deterministic 'reproduction with instrumentation' was the tracing itself crashing in strlen() on an integer consumed as %s. Validation: full api_test passes with -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_RUNTIME_CHECKS=1 (the minimal-test configuration) as well as plain RelWithDebInfo and Debug; TSAN run of the arrow/CSR suite is clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #881.
Root cause
ArrowResultCollectorand the regularResultCollectorboth declarestatic constexpr PhysicalOperatorType type_ = PhysicalOperatorType::RESULT_COLLECTOR.In
PlanMapper::getPhysicalPlan()the plan-root marker introduced by #870/#877 work was applied via an operator-type check:For every arrow query (
resultType=ARROW), the newly createdArrowResultCollectormatched that check and was then downcast toResultCollector, which it is not:RUNTIME_CHECKS(or any!NDEBUGbuild),dynamic_cast_checkedasserts and throws. EveryqueryAsArrow()execution failed and returned an errorMaterializedQueryResultwhose iterator is null. Tests callinggetNextArrowChunk()without checkingisSuccess()then SIGSEGV inFactorizedTableIterator::hasNext— theFatal signal 11in the linux minimal-test job. The remaining tests fail ondynamic_cast<ArrowQueryResult*>(result.get()) == nullptrandentry.relCsrResults[0] == nullptr(PROJECT_GRAPHmaterialization also runs throughqueryAsArrow).NDEBUG(plain release), the checked cast is areinterpret_castandsetResultExposedToClient()writes a byte atResultCollector::internalResultTable's offset (192), which in anArrowResultCollectorobject islocalState.batchIndex— UB on every arrow query.The minimal-test job builds with
-DENABLE_RUNTIME_CHECKS=1, which made the failure deterministic across all 10 arrow/CSR tests — while local (non-RUNTIME_CHECKS) runs looked healthy, which is why the issue looked like a timing-dependent race.Fix
Mark the client-facing result table on the freshly created regular
ResultCollectorinside the non-arrow branch of theif/else, instead of matching on the shared operator-type enum afterwards.Also re-enables the ten tests skipped in 520abda.
Note on the earlier investigation
The "corrupted task clone" evidence in the issue (
this== sharedState, garbage batch indices, CSR metadata present for CSR-free queries) was an artefact of the temporaryLBUG_ARROW_DEBUGtracing: itsfprintfmacro appended the thread-id string after the caller's varargs while the format consumed%sfirst, shifting every printed value by one slot. The deterministic "reproduction with instrumentation" was the tracing itself crashing instrlen()on an integer consumed as%s. With corrected tracing, all collector state is healthy up to the crash.Validation
api_test(304 tests) passes with-DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_RUNTIME_CHECKS=1(the minimal-test configuration), plainRelWithDebInfo, andDebugbuilds.TSANrun of the arrow/CSR suite is clean.