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

Fix predicate failure for negative offset value on _timestamp column #13170

Merged
merged 2 commits into from
Aug 9, 2022
Merged

Fix predicate failure for negative offset value on _timestamp column #13170

merged 2 commits into from
Aug 9, 2022

Conversation

harris233
Copy link
Contributor

@harris233 harris233 commented Jul 14, 2022 •
edited by ebyhr
Loading

Description

Is this change a fix, improvement, new feature, refactoring, or other?
A fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)
Kafka connector

How would you describe this change to a non-technical end user or system administrator?
Fix predicate failure for negative offset value on _timestamp column

Related issues, pull requests, and links

Fixes #13167

Documentation

(x) No documentation is needed.

Release notes

(x) Release notes entries required with the following suggested text:

Issue:#13167

@harris233 harris233 mentioned this pull request Jul 14, 2022
Closed
ebyhr
ebyhr reviewed Jul 14, 2022
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for you contribution! Please submit CLA if you haven't sent it https://github.com/trinodb/cla.

Could you please add a test to TestKafkaIntegrationPushDown?
Also, I would recommend improving the commit title - we can't understand what's the issue from "small bug".

Praveen2112
Praveen2112 reviewed Jul 14, 2022
View reviewed changes
plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/KafkaFilterManager.java Outdated
@@ -129,11 +129,15 @@ public KafkaFilteringResult getKafkaFilterResult(
if (offsetTimestampRanged.isPresent()) {
try (KafkaConsumer<byte[], byte[]> kafkaConsumer = consumerFactory.create(session)) {
Optional<Range> finalOffsetTimestampRanged = offsetTimestampRanged;
partitionBeginOffsets = overridePartitionBeginOffsets(partitionBeginOffsets,
partition -> findOffsetsForTimestampGreaterOrEqual(kafkaConsumer, partition, finalOffsetTimestampRanged.get().getBegin()));
if (offsetTimestampRanged.get().getBegin() >= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add some tests to check if it works as expected ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I added some test cases in TestKafkaIntegrationPushDown.

@harris233 harris233 changed the title Fix small bug when we use trino to select kafka with timestamp Fix bug when we use trino to select kafka with timestamp Jul 15, 2022
@harris233
Copy link
Contributor Author

Thanks for you contribution! Please submit CLA if you haven't sent it https://github.com/trinodb/cla.

Could you please add a test to TestKafkaIntegrationPushDown? Also, I would recommend improving the commit title - we can't understand what's the issue from "small bug".
OK, I sent cla to cla@trino.io just now. I have added some test cases and updated the commit title.

@harris233 harris233 requested a review from ebyhr July 15, 2022 05:21
@cla-bot cla-bot bot added the cla-signed label Jul 21, 2022
@harris233 harris233 force-pushed the bugfix-kakfa-timestamp-error branch from 016441c to 3738407 Compare July 21, 2022 02:09
@harris233
Copy link
Contributor Author

harris233 commented Jul 21, 2022 •
edited
Loading

Hello, I have signed CLA and got it.Please review again @ebyhr @Praveen2112

@harris233 harris233 requested a review from Praveen2112 July 21, 2022 04:53
@trinodb trinodb deleted a comment from cla-bot bot Jul 22, 2022
@trinodb trinodb deleted a comment from cla-bot bot Jul 22, 2022
@trinodb trinodb deleted a comment from cla-bot bot Jul 22, 2022
@trinodb trinodb deleted a comment from cla-bot bot Jul 22, 2022
@trinodb trinodb deleted a comment from cla-bot bot Jul 22, 2022
ebyhr
ebyhr reviewed Jul 22, 2022
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Could you fix the commit title likes below sentence?

Fix predicate failure for negative offset value on _timestamp column

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/KafkaFilterManager.java Outdated Show resolved Hide resolved
plugin/trino-kafka/src/test/java/io/trino/plugin/kafka/TestKafkaIntegrationPushDown.java Outdated Show resolved Hide resolved
@harris233 harris233 changed the title Fix bug when we use trino to select kafka with timestamp Fix predicate failure for negative offset value on _timestamp column Jul 26, 2022
@harris233 harris233 force-pushed the bugfix-kakfa-timestamp-error branch from 715f013 to f4d2bad Compare July 26, 2022 02:06
@harris233
Copy link
Contributor Author

harris233 commented Jul 26, 2022 •
edited
Loading

Could you fix the commit title likes below sentence?

Fix predicate failure for negative offset value on _timestamp column

OK, I changed to Fix predicate failure for negative offset value on _timestamp column @ebyhr

@harris233 harris233 force-pushed the bugfix-kakfa-timestamp-error branch from f4d2bad to cac3dc4 Compare July 26, 2022 02:16
@harris233 harris233 requested a review from ebyhr July 26, 2022 03:11
@harris233 harris233 force-pushed the bugfix-kakfa-timestamp-error branch 2 times, most recently from 5b24559 to 7c5648e Compare August 8, 2022 03:41
@harris233 harris233 requested a review from ebyhr August 8, 2022 04:54
@ebyhr
Copy link
Member

ebyhr commented Aug 9, 2022

@harris233 Please rebase instead of merge to follow upstream.

@harris233 harris233 force-pushed the bugfix-kakfa-timestamp-error branch from 65e7124 to 2d32f22 Compare August 9, 2022 02:11
@harris233
Copy link
Contributor Author

@harris233 Please rebase instead of merge to follow upstream.

OK, I rebased it.

@harris233 harris233 force-pushed the bugfix-kakfa-timestamp-error branch from 2d32f22 to d775527 Compare August 9, 2022 03:09
@harris233 harris233 force-pushed the bugfix-kakfa-timestamp-error branch from 0305df2 to d6fb5d5 Compare August 9, 2022 05:26
@harris233 harris233 requested a review from ebyhr August 9, 2022 06:21
@ebyhr ebyhr merged commit 57714f7 into trinodb:master Aug 9, 2022
@ebyhr
Copy link
Member

ebyhr commented Aug 9, 2022

Merged, thanks!

@ebyhr ebyhr mentioned this pull request Aug 9, 2022
Closed
@github-actions github-actions bot added this to the 393 milestone Aug 9, 2022
@colebow colebow mentioned this pull request Aug 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Praveen2112 Praveen2112 Awaiting requested review from Praveen2112

@ebyhr ebyhr Awaiting requested review from ebyhr

Assignees
No one assigned
Labels
cla-signed
Milestone
393
Development

Successfully merging this pull request may close these issues.

Fix predicate failure for negative offset value on _timestamp column
3 participants
@harris233 @ebyhr @Praveen2112