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.

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

Implement BaseConnectorTest in Pinot connector #13059

Closed
ebyhr opened this issue Jul 1, 2022 · 8 comments · Fixed by #17582
Closed

Implement BaseConnectorTest in Pinot connector #13059

ebyhr opened this issue Jul 1, 2022 · 8 comments · Fixed by #17582
Assignees
Labels
good first issue Good for newcomers test

Comments

@ebyhr
Copy link
Member

ebyhr commented Jul 1, 2022

  1. AbstractPinotIntegrationSmokeTest should extends BaseConnectorSmokeTest
  2. Add a new TestPinotConnectorTest extending BaseConnectorTest
@ebyhr ebyhr added good first issue Good for newcomers test labels Jul 1, 2022
@ntantri
Copy link
Member

ntantri commented Jul 29, 2022

Hi, @ebyhr is this pertaining to just extending the BaseConnectorTest in https://github.com/trinodb/trino/blob/master/plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationSmokeTest.java#L96 ?

Can you provide more description on the actual expectation of this?

@ebyhr
Copy link
Member Author

ebyhr commented Jul 29, 2022

@tan31989 Updated the description.

@ntantri
Copy link
Member

ntantri commented Aug 2, 2022

Hi @ebyhr, I was trying to extend BaseConnectorSmokeTest for AbstractPinotIntegrationSmokeTest, but then realised that the SQL assertions in BaseConnectorSmokeTest are designed for a normal SQL queries, whereas, AbstractPinotIntegrationSmokeTest has tables/SQL's that require JSON specific queries. For instance, pinot specifically has Kafka that creates records in JSON and is expected to be fetched with JSON queries.

Is the intent to remove all those queries and freshly create new tables/data that match queries in BaseConnectorSmokeTest?

@ebyhr
Copy link
Member Author

ebyhr commented Aug 2, 2022

create new tables/data that match queries in BaseConnectorSmokeTest?

Correct, but please don't remove existing tests.

@ntantri
Copy link
Member

ntantri commented Aug 13, 2022

@ebyhr have created one PR for (adding you as a reviewer), please do review it.

AbstractPinotIntegrationSmokeTest should extends BaseConnectorSmokeTest

Working on the second one as a separate PR to make sure it's easy to develop.

@ntantri ntantri self-assigned this Aug 13, 2022
@ntantri
Copy link
Member

ntantri commented Sep 28, 2022

@ebyhr , I have been scratching my head around a problem to write tests for TestPinotConnectorTest for a while now.

Need some assistance before I proceed further.

  • BaseConnectorTest has a table orders, that has a column for date
  • Pinot support date transformation using some functions, mentioned here and here

But,

  • The schema builder used to ingest data is org.apache.avro.SchemaBuilder, example, see this SchemaBuilder usage
  • This SchemaBuilder doesn't support a TimeStamp type that Pinot supports. Not sure how to make the tests for orders pass in this case.

Any input on what is acceptable in this case?

My thoughts, there are two ways, either:

  • Figure out some other way to ingest data that allows timestamp column (might require me to change kafka.sendMessages to accept something other than Avro

or

  • Ignore all tests for orders.

@ebyhr
Copy link
Member Author

ebyhr commented Sep 28, 2022

@xiangfu0 @elonazoulay Can you help to setup orders table?

@ntantri
Copy link
Member

ntantri commented Oct 20, 2022

@xiangfu0 @elonazoulay any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers test
Development

Successfully merging a pull request may close this issue.

3 participants