-
Notifications
You must be signed in to change notification settings - Fork 268
Fix Iceberg view test failures in CICD #14082
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
build |
Greptile SummaryThis PR fixes Iceberg view test failures that occurred with Iceberg 1.9.x by restricting view tests to only run with REST catalog and ensuring test tables have data before creating views. Key Changes:
The fix is well-structured and aligns with existing patterns in the codebase. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Test as test_iceberg_view
participant Factory as spark_tmp_table_factory
participant Helper as create_iceberg_table
participant Session as with_cpu_session
participant Spark as SparkSession
Test->>Factory: get_full_table_name()
Factory-->>Test: table_name
Test->>Helper: create_iceberg_table(table_name)
Helper->>Session: with_cpu_session(set_iceberg_table)
Session->>Spark: Generate schema from df_gen
Session->>Spark: CREATE TABLE ... USING ICEBERG
Spark-->>Session: Table created (empty)
Session-->>Helper: Done
Helper-->>Test: table_name
Note over Test: NEW: Insert data step added in PR
Test->>Session: with_cpu_session(insert_data)
Session->>Spark: gen_df() with iceberg_gens_list
Session->>Spark: df.writeTo(table_name).append()
Spark-->>Session: Data inserted
Session-->>Test: Done
Test->>Session: with_cpu_session(setup_iceberg_view)
Session->>Spark: CREATE VIEW view_name AS view_sql
Spark-->>Session: View created
Session-->>Test: Done
Test->>Test: assert_gpu_and_cpu_are_equal_collect()
Test->>Spark: SELECT * FROM view_name
Spark-->>Test: Results verified
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (1)
-
integration_tests/src/main/python/iceberg/iceberg_view_test.py, line 73-74 (link)style: The temporary Spark view created here is never dropped and could accumulate across test runs. Consider adding cleanup after table creation.
1 file reviewed, 1 comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinkg that maybe another solution would be to run it on rest catalog only?
|
Now |
a25584c to
2940050
Compare
Signed-off-by: Chong Gao <res_life@163.com>
|
build |
Greptile's behavior is changing!From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
abellina
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
build |
|
Going to merge this to ensure today's run can cover this change, thanks |
Fixes #14075.
Description
why pipelines failed
For pipelines:
rapids_it-iceberg-s3tables-devandrapids_it-iceberg-devpipeline 1.6.x passed, but 1.9.x failed.
From Iceberg 1.9.x, using hadoop type with SparkSessionCatalog will fail with:
"Creating a view is not supported by catalog: spark_catalog"
Iceberg 1.9.x changed the behavior.
For S3TableCatalog, it also does not support view, tested, trying to create a view in
s3_catalog.namespacealso fails with: "Creating a view is not supported by catalog"how to fix
Only test
restcatalog, skiphadoopands3tablecatalog.restcatalog supports view.Checklists
(Please explain in the PR description how the new code paths are tested, such as names of the new/existing tests that cover them.)