Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable Delta Write fallback tests on Databricks 12.2 [databricks] #8628

Merged
merged 6 commits into from
Jun 30, 2023
Merged

Enable Delta Write fallback tests on Databricks 12.2 [databricks] #8628

merged 6 commits into from
Jun 30, 2023

Conversation

andygrove
Copy link
Contributor

Part of #8423

The Delta Write fallback tests were expecting to see ExecutedCommandExec in the final plan, but the plan in Databricks 12.2 has DataWritingCommandExec instead.

This PR updates the tests and enables them for Databricks 12.2

@andygrove andygrove added the test Only impacts tests label Jun 28, 2023
@andygrove andygrove self-assigned this Jun 28, 2023
@andygrove andygrove changed the title WIP: Enable Delta Write fallback tests on Databricks 12.2 WIP: Enable Delta Write fallback tests on Databricks 12.2 [databricks] Jun 28, 2023
@andygrove
revert metrics change
cd5df84 
Signed-off-by: Andy Grove <andygrove@nvidia.com>
@andygrove
Copy link
Contributor Author

build

@andygrove andygrove marked this pull request as ready for review June 29, 2023 12:15
@andygrove andygrove changed the title WIP: Enable Delta Write fallback tests on Databricks 12.2 [databricks] Enable Delta Write fallback tests on Databricks 12.2 [databricks] Jun 29, 2023
jlowe
jlowe reviewed Jun 29, 2023
View reviewed changes
integration_tests/src/main/python/delta_lake_write_test.py Outdated
Comment on lines 64 to 65
# we remove deletion vector metrics because we do not support those yet
# see https://github.com/NVIDIA/spark-rapids/issues/8554
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there's two phases to supporting deletion vectors. There's full support which is tracked by #8554, but before that is completed we should be reporting the metrics (always 0) in the delta log stats just like the CPU does when not using deletion vectors. I could see shipping without full deletion support, but we should be adding these deletion metrics even when we don't support them being non-zero. I'm OK with that being a followup, but IMO having the metric is a P0 for the release while the full support issue is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should not have been in this PR. I have copied your comment into #8624 (comment) where we can discuss more. I have removed this change from this PR since this PR is all about falling back to CPU.

@andygrove andygrove mentioned this pull request Jun 29, 2023
Merged
jlowe
jlowe previously approved these changes Jun 29, 2023
View reviewed changes
@jlowe
Copy link
Member

jlowe commented Jun 29, 2023

build

@andygrove
Copy link
Contributor Author

build

@andygrove andygrove merged commit 68365a2 into NVIDIA:branch-23.08 Jun 30, 2023
@andygrove andygrove deleted the dbr-fallback branch June 30, 2023 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlowe jlowe jlowe approved these changes

Assignees

@andygrove andygrove

Labels
test Only impacts tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@andygrove @jlowe