Repository navigation
[test](fe) Cover format=avro table functions failing during analysis - #69849
Merged
Merged
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#69601, apache#64699 Problem Summary: apache#69601 removed standalone Avro file reads and made FE reject format=avro while it parses file format properties, with the message "Avro file reads are no longer supported". Its unit test calls FileFormatProperties.createFileFormatProperties directly; nothing checks that a table function reaches that check before it validates storage properties or lists files, so reordering a constructor would turn the rejection back into a storage or BE error. Before apache#69601 such a query failed in BE with "Not supported file format in fetch table schema: 12" (branch-4.2 still reads Avro through the JNI scanner that apache#64699 fixed). This adds a regression suite that runs s3() and hdfs() with format=avro and AVRO against made-up locations and expects the analysis-time message, and a unit test that constructs S3TableValuedFunction and HdfsTableValuedFunction with that format. ### Release note None ### Check List (For Author) - Test: Regression test (test_tvf_avro_format_rejected) and Unit Test (UnsupportedExternalReadsTest) - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed all changes in both test files at 96491d6. No actionable findings or existing blocking comments were identified.
- Goal and proof: the new constructor test and four SQL negative cases cover S3/HDFS Avro rejection before storage validation or file listing. The factory, constructor order and SQL error propagation agree with the asserted message.
- Scope: focused test additions and necessary imports; no production code changes.
- Concurrency and locks: local fixture maps, synchronous regression actions, and no shared-state changes or new locking.
- Lifecycle and initialization: these inputs reject before storage adapters or handles are acquired; no resource property triggers the earlier Env lookup, and rejection precedes unit-test shortcuts.
- Configuration: no settings added or changed.
- Compatibility: no runtime symbols, storage formats or upgrade behavior changed.
- Thrift/protobuf: no definitions changed; field-ID checks are inapplicable.
- Parallel paths: both targeted TVFs are covered. Regression exercises both avro and AVRO for each; existing factory tests also cover case-insensitive rejection.
- Conditional checks: the only added loop deterministically enumerates the two format values.
- Coverage: JUnit 5 assertions check the correct unchecked AnalysisException. The regression suite follows discovery/group conventions and requires an actual exception containing the specific Avro diagnostic, so success and unrelated storage errors fail.
- Test results: no snapshots were added or modified. Exception-only cases need neither row ordering nor .out files, and they create no tables.
- Observability: the existing explicit diagnostic and assertion failures suffice for this test-only change.
- Transactions and persistence: no changes to journal/replay, failover, visibility or delete-bitmap behavior.
- Writes and atomicity: no data mutations or transaction operations introduced.
- FE/BE propagation: no variables added; the intended rejection occurs during FE analysis before schema RPC or BE execution.
- Performance and memory: tiny bounded test allocations and four early-failing statements; no production overhead introduced.
- Other issues: rechecked exception wrapping, global test flags, superclass initialization and immediate closure execution; no additional issue found.
No additional user focus was supplied. There were no existing inline threads to carry forward. Review converged after one round: both the complete reviewer and the independent storage-order risk reviewer returned NO_NEW_VALUABLE_FINDINGS, and the main verification/final sweep left no unresolved candidates or static coverage gaps.
Validation limit: static review only. No builds, tests, benchmarks or runtime checks were run; the PR author's reported test results were not independently reproduced.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28408 ms |
Contributor
TPC-DS: Total hot run time: 150715 ms |
Contributor
ClickBench: Total hot run time: 24.44 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
… in the suite comment ### What problem does this PR solve? Issue Number: None Related PR: apache#61141, apache#69601 Problem Summary: The header comment of test_tvf_avro_format_rejected said that apache#69601 removed the BE Avro reader. On master the reader was removed by apache#61141; apache#69601 made FE reject format=avro during analysis and added a BE-side guard. Correct the PR number in the comment. Comment only. ### Release note None ### Check List (For Author) - Test: No need to test (comment-only change); the suite still passes the offline Groovy parse check - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Gabriel39
approved these changes
Oct 31, 2026
Contributor
There was a problem hiding this comment.
Reviewed PR 69849 at exact head 202742a against base 2a4191c. The change is test-only and adds FE unit coverage plus a regression suite for S3/HDFS table functions with format=avro and FORMAT=AVRO.
The constructor paths were traced through ExternalFileTableValuedFunction.parseCommonProperties: FileFormatProperties rejects Avro with the Nereids AnalysisException and exact message before StorageAdapter binding, URI validation, or file listing. The JUnit assertions therefore use the correct exception type, and the regression cases are hermetic and use the established suite/test/exception DSL with substring matching. Both changed paths, imports, case coverage, suite discovery, and exception handling were independently checked. No locking, RPC, cloud visible-version, or production behavior path is changed. There are no existing inline review threads or blocking IDs, and no additional user focus points were supplied.
No builds or tests were executed because this invocation is static-only; the review evidence is from the exact diff and traced source/harness paths. No actionable findings remain. Review converged in round 1.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28411 ms |
Contributor
TPC-DS: Total hot run time: 151201 ms |
Contributor
ClickBench: Total hot run time: 24.36 s |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
924060929
approved these changes
Oct 31, 2026
924060929
left a comment
924060929
left a comment
Contributor
There was a problem hiding this comment.
This test-only PR pins the existing FE rejection of Avro S3/HDFS table functions before storage validation or file access. Both constructors reach parseCommonProperties and the file-format factory first; SQL exception wrapping preserves the asserted diagnostic.
Reviewed the complete diff at 202742a against merge-base 2a4191c, including constructor paths and regression assertion semantics. The tests reject successful queries and unrelated errors, cover both functions and both format spellings, and introduce no production changes. No actionable P0/P1/P2 findings. APPROVE within the FE static-review scope.
The supplied records include Gabriel39's approval and an AI review with no findings at this head; there are no existing inline comments. P0 Regression, FE UT, External Regression, and compilation are reported successful, while check_coverage_fe failed. Required-check completeness is unknown, so this approval does not establish merge readiness. No builds or tests were run during this review.
Automated FE-focused static review (GPT-6.1 Sol, medium). No build or tests were run.
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.
What problem does this PR solve?
Issue Number: None
Related PR: #69601, #64699
Problem Summary:
In short. #69601 made FE reject
format=avrowhile it parses file format properties. This PR adds end-to-end coverage for that: a regression suite that runss3()andhdfs()withformat=avroand checks that they fail during analysis, and a unit test for the two table-function constructors. Test only, no code change.Background. Master no longer reads standalone Avro files: #61141 removed the BE reader, #65733 the scanner module, and #69601 made
FileFormatProperties.createFileFormatPropertiesthrowAvro file reads are no longer supported. Before #69601, FE accepted the format and the query failed later in BE withNot supported file format in fetch table schema: 12. branch-4.2 still reads Avro through the JNI scanner (#64699 fixed a crash in it), so users who upgrade from 4.x with Avro table functions or loads get this error.What was missing. #69601's unit test calls
createFileFormatPropertiesdirectly. Nothing checks that a table function reaches that check before it validates storage properties or lists files. If a constructor validated the location first,format=avroagainst an unreachable location would report a storage error instead, and no test would notice.What this PR adds.
external_table_p0/tvf/test_tvf_avro_format_rejected:s3()andhdfs()withformatset toavroand toAVRO, against a made-up bucket, made-up credentials and an HDFS address nothing listens on. Each call must fail withAvro file reads are no longer supported.UnsupportedExternalReadsTest#testAvroTableFunctionsAreRejectedBeforeStorageAccess: constructsS3TableValuedFunctionandHdfsTableValuedFunctionwithformat=avro/AVROand checks the same message.Results. On master
2a4191c1dc9with this change, all four statements of the suite fail during analysis withCan not build s3(): Avro file reads are no longer supported/Can not build hdfs(): ..., and the unit test passes. Before #69601 the sames3()call would have gone on to validate and list the made-up location, so the suite pins the order as well as the message.Release note
None
Check List (For Author)
run-regression-test.sh --run -d external_table_p0/tvf -s test_tvf_avro_format_rejected: passes on a local cluster whose FE is built from this branch.run-fe-ut.sh --run org.apache.doris.datasource.UnsupportedExternalReadsTest: 3 of 3 pass; fe-core checkstyle passes.Check List (For Reviewer who merge this PR)