Skip to content

[Pulsar sql] Support keyValue Schema add integration test. #9388

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
Merged
sijie merged 6 commits into from
Feb 22, 2021
Merged

[Pulsar sql] Support keyValue Schema add integration test. #9388

sijie merged 6 commits into from
Feb 22, 2021

Conversation

@congbobo184
Copy link
Contributor

@congbobo184 congbobo184 commented Feb 1, 2021 •
edited
Loading

Motivation

Add the test for pulsar-sql support keyValue schema

implement

Add the test

Verifying this change

Add the test for it

Does this pull request potentially affect one of the following parts:
If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

sijie
sijie requested changes Feb 1, 2021
View reviewed changes
Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

@congbobo184 Can you add an integration test to test KeyValue schema with both separate and inline modes?

@sijie sijie added this to the 2.8.0 milestone Feb 1, 2021
@congbobo184
Copy link
Contributor Author

congbobo184 commented Feb 1, 2021

@congbobo184 Can you add an integration test to test KeyValue schema with both separate and inline modes?

OK

Merge branch 'master' into congbobo184_pulsr_sql_keyvalue_separated_s…
8536557 
…upport

# Conflicts:
#	pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/AvroSchemaHandler.java
#	pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/KeyValueSchemaHandler.java
#	pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/PulsarRecordCursor.java
#	pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/PulsarSchemaHandlers.java
#	pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/PulsarSqlSchemaInfoProvider.java
#	pulsar-sql/presto-pulsar/src/test/java/org/apache/pulsar/sql/presto/TestPulsarKeyValueSchemaHandler.java
#	pulsar-sql/presto-pulsar/src/test/java/org/apache/pulsar/sql/presto/TestPulsarPrimitiveSchemaHandler.java
@congbobo184
Copy link
Contributor Author

congbobo184 commented Feb 2, 2021

#8422 has solve this problem, so this PR only add the test

@hnail
Copy link
Contributor

hnail commented Feb 2, 2021

#8422 has solve this problem, so this PR only add the test

I added TestPulsarRecordCursor#TestKeyValueStructSchema and TestPulsarRecordCursor#TestKeyValuePrimitiveSchema in #8422 , which contains KeyValueEncodingType.INLINE and KeyValueEncodingType.SEPARATED modes unit test , but lack of integration test ?

@congbobo184
Copy link
Contributor Author

congbobo184 commented Feb 2, 2021

#8422 has solve this problem, so this PR only add the test

I added TestPulsarRecordCursor#TestKeyValueStructSchema and TestPulsarRecordCursor#TestKeyValuePrimitiveSchema in #8422 , which contains KeyValueEncodingType.INLINE and KeyValueEncodingType.SEPARATED modes unit test , but lack of integration test ?

Yes, it lack integration test. If you want to add the test, i will close this PR.

@hnail
Copy link
Contributor

hnail commented Feb 2, 2021 •
edited
Loading

#8422 has solve this problem, so this PR only add the test

I added TestPulsarRecordCursor#TestKeyValueStructSchema and TestPulsarRecordCursor#TestKeyValuePrimitiveSchema in #8422 , which contains KeyValueEncodingType.INLINE and KeyValueEncodingType.SEPARATED modes unit test , but lack of integration test ?

Yes, it lack integration test. If you want to add the test, i will close this PR.

#8422 has solve this problem, so this PR only add the test

I added TestPulsarRecordCursor#TestKeyValueStructSchema and TestPulsarRecordCursor#TestKeyValuePrimitiveSchema in #8422 , which contains KeyValueEncodingType.INLINE and KeyValueEncodingType.SEPARATED modes unit test , but lack of integration test ?

Yes, it lack integration test. If you want to add the test, i will close this PR.

thanks for replay , I'm not working the integration tests.

@congbobo184
Copy link
Contributor Author

congbobo184 commented Feb 2, 2021 •
edited
Loading

@hnail OK, i will work on this pr. thanks reply.

@congbobo184
Copy link
Contributor Author

congbobo184 commented Feb 19, 2021

/pulsarbot run-failure-checks

@congbobo184
Copy link
Contributor Author

congbobo184 commented Feb 20, 2021

/pulsarbot run-failure-checks

@congbobo184 congbobo184 changed the title [Pulsar sql] Support keyValue Schema separate mode. [Pulsar sql] Support keyValue Schema add integration test. Feb 20, 2021
@codelipenghui
Copy link
Contributor

codelipenghui commented Feb 22, 2021

@sijie Please help take a look at this PR again. @congbobo184 Could you please check the branch-2.7? I think branch-2.7 does not work. Since we can't cherry-pick #8422, could you please create a PR to fix the branch-2.7?

@sijie sijie merged commit 18eb2e5 into apache:master Feb 22, 2021
@codelipenghui
Copy link
Contributor

codelipenghui commented Feb 25, 2021

#9685 is fixing the branch-2.7

@codelipenghui codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sijie sijie sijie approved these changes

@jiazhai jiazhai jiazhai approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

Assignees

@congbobo184 congbobo184

Labels

area/sql Pulsar SQL related features cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.1

Projects

None yet

Milestone

2.8.0

Development

Successfully merging this pull request may close these issues.

5 participants

@congbobo184 @hnail @codelipenghui @sijie @jiazhai