Skip to content

Move limit/topN pushdown tests to base connector test #26521

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
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Move limit/topN pushdown tests to base connector test #26521

wants to merge 4 commits into from

Conversation

chenjian2664
Copy link
Contributor

@chenjian2664 chenjian2664 commented Aug 31, 2025 •
edited
Loading

Description

Previously, there were no tests for non-JDBC connectors to verify limit and TopN pushdown.

Additional context and related issues

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

## Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Aug 31, 2025
@chenjian2664 chenjian2664 marked this pull request as draft August 31, 2025 10:05
@github-actions github-actions bot added mongodb MongoDB connector cassandra Cassandra connector opensearch OpenSearch connector hudi Hudi connector iceberg Iceberg connector hive Hive connector elasticsearch Elasticsearch connector kafka Kafka connector pinot Pinot connector redis Redis connector labels Aug 31, 2025
@chenjian2664 chenjian2664 force-pushed the limit-topn-test-update branch 3 times, most recently from c22a9e3 to 91e40e8 Compare September 1, 2025 07:02
@github-actions github-actions bot added the memory Memory connector label Sep 1, 2025
@chenjian2664 chenjian2664 force-pushed the limit-topn-test-update branch 3 times, most recently from 867b1a9 to bcbe54e Compare September 1, 2025 17:51
@chenjian2664 chenjian2664 changed the title Move limit test to base connector test Move limit/topN pushdown tests to base connector test Sep 2, 2025
@chenjian2664 chenjian2664 marked this pull request as ready for review September 2, 2025 03:38
@ebyhr
Copy link
Member

ebyhr commented Sep 2, 2025

/test-with-secrets sha=bcbe54ef19fc100caed0302bab2d471442acd988

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 2, 2025 •
edited
Loading

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/17392797220

@ebyhr
Copy link
Member

ebyhr commented Sep 2, 2025

Could you fix BigQuery CI failure?

Error:    TestBigQueryAvroConnectorTest>BaseConnectorTest.testLimitPushdown:434 Plan does not match, expected [

- node(OutputNode)
    - node(TableScanNode)

] but found [

Output[columnNames = [name]]
│   Layout: [name:varchar]
└─ Limit[count = 30]
   │   Layout: [name:varchar]
   └─ LocalExchange[partitioning = SINGLE]
      │   Layout: [name:varchar]
      └─ RemoteExchange[type = GATHER]
         │   Layout: [name:varchar]
         └─ LimitPartial[count = 30]
            │   Layout: [name:varchar]
            └─ TableScan[table = bigquery:sep-bq-cicd.tpch.nation:TABLE columns=[name:varchar]]
                   Layout: [name:varchar]
                   name := name:varchar

@github-actions github-actions bot added the bigquery BigQuery connector label Sep 2, 2025
@chenjian2664
Copy link
Contributor Author

@ebyhr could you help to take a look again?

@ebyhr
Copy link
Member

ebyhr commented Sep 2, 2025

/test-with-secrets sha=7b6415d85db63ba4eef0dd7260af5be3faf9d74f

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 2, 2025 •
edited
Loading

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/17418919957

@chenjian2664
Copy link
Contributor Author

the ci failure Error: The action has timed out. not relates the change

@chenjian2664
Copy link
Contributor Author

@ebyhr Could you please take a look again?

ebyhr
ebyhr reviewed Sep 5, 2025
View reviewed changes
@github-actions github-actions bot added the druid Druid connector label Sep 5, 2025
@chenjian2664 chenjian2664 force-pushed the limit-topn-test-update branch from 0a4fba3 to 310caff Compare September 5, 2025 05:52
@chenjian2664 chenjian2664 force-pushed the limit-topn-test-update branch from 436f246 to 33d9184 Compare September 5, 2025 13:25
@chenjian2664
Move limit pushdown test into BaseConnectorTest
f82c5b6 
Previously, there was no test to exercise limit pushdown behavior
for non-JDBC connectors, regardless of whether they declare support
for the `SUPPORTS_LIMIT_PUSHDOWN` TestingConnectorBehavior.
Aslo fix the bug in Memory connector test where the connector
supports limit pushdown.
@chenjian2664
Move topN pushdown test into BaseConnectorTest
3e4a77b 
Previously, there was no test to exercise topN pushdown behavior
for non-JDBC connectors, regardless of whether they declare support
for the `SUPPORTS_TOPN_PUSHDOWN` TestingConnectorBehavior.
@chenjian2664 chenjian2664 force-pushed the limit-topn-test-update branch from 33d9184 to 3e4a77b Compare September 6, 2025 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebyhr ebyhr ebyhr left review comments

@Praveen2112 Praveen2112 Awaiting requested review from Praveen2112

@hashhar hashhar Awaiting requested review from hashhar

Assignees
No one assigned
Labels
bigquery BigQuery connector cassandra Cassandra connector cla-signed druid Druid connector elasticsearch Elasticsearch connector hive Hive connector hudi Hudi connector iceberg Iceberg connector kafka Kafka connector memory Memory connector mongodb MongoDB connector opensearch OpenSearch connector pinot Pinot connector redis Redis connector
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chenjian2664 @ebyhr