Skip to content

[SPARK-58555] Add new tests for JDBC DS Join pushdown - #57714

Open
urosstan-db wants to merge 2 commits into
apache:masterfrom
urosstan-db:urosstan-db/additional-join-tests
Open

[SPARK-58555] Add new tests for JDBC DS Join pushdown#57714
urosstan-db wants to merge 2 commits into
apache:masterfrom
urosstan-db:urosstan-db/additional-join-tests

Conversation

@urosstan-db

@urosstan-db urosstan-db commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

  • Add two new tests in suite that verifies behaviour of JDBC Data Source join pushdown

Why are the changes needed?

  • Optimizer rules may be reordered or new rule may be added, so join on top of aggregates may not be pushed down anymore. In this way, we make feature coverage more robust.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Test addtion/NA

Was this patch authored or co-authored using generative AI tooling?

No

@urosstan-db urosstan-db changed the title Add new tests Add new tests for JDBC Join pushdown Aug 3, 2026
@urosstan-db urosstan-db changed the title Add new tests for JDBC Join pushdown Add new tests for JDBC DS Join pushdown Aug 3, 2026
@urosstan-db
urosstan-db force-pushed the urosstan-db/additional-join-tests branch from 77473cf to c0e1a12 Compare August 3, 2026 09:09
@urosstan-db
urosstan-db force-pushed the urosstan-db/additional-join-tests branch from c0e1a12 to 6d82897 Compare August 3, 2026 09:09
sql(sqlQuery).collect().toSeq
}

assert(rowsNoPushdown.nonEmpty)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These asserts don't seem to be doing much at the moment, please consider removing them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added it as additional guard against modifications on test dataset, so I would like to preserve some guard against that (to make our test still relevant in the future), it is not requirement ofc. Do you maybe suggest we make assertion on entire result then, or asserting on number of rows, etc? Or totally removing it?

sql(sqlQuery).collect().toSeq
}

assert(rowsNoPushdown.nonEmpty)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@uros-b uros-b left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add a Spark Jira ticket and component tag to the PR title.

@urosstan-db urosstan-db changed the title Add new tests for JDBC DS Join pushdown [SPARK-58555] Add new tests for JDBC DS Join pushdown Aug 4, 2026
@urosstan-db

Copy link
Copy Markdown
Contributor Author

Please add a Spark Jira ticket and component tag to the PR title.

Added now, sorry for not having the one initially

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants