Skip to content

[pinot connector]Support pinot connector to read DataTableV4 bytes#19942

Merged
ebyhr merged 1 commit intotrinodb:masterfrom
xiangfu0:fixing-pinot-bytes-read
Nov 30, 2023
Merged

[pinot connector]Support pinot connector to read DataTableV4 bytes#19942
ebyhr merged 1 commit intotrinodb:masterfrom
xiangfu0:fixing-pinot-bytes-read

Conversation

@xiangfu0
Copy link
Contributor

@xiangfu0 xiangfu0 commented Nov 28, 2023

Description

Pinot DataTableV4 has a different implementation for reading Bytes:

  • V2/V3 uses hex string to represent bytes,
  • V4 directly uses var length bytes encoding.

This PR will check the implementation of DataTable then pick the right method to read it.

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:

@cla-bot cla-bot bot added the cla-signed label Nov 28, 2023
@xiangfu0 xiangfu0 force-pushed the fixing-pinot-bytes-read branch 2 times, most recently from dd82a55 to 5006cc3 Compare November 29, 2023 08:32
@xiangfu0 xiangfu0 requested a review from ebyhr November 29, 2023 19:47
Copy link
Member

@ebyhr ebyhr Nov 29, 2023

Choose a reason for hiding this comment

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

This behavior is weird to me. There's org.apache.pinot.common.datatable.DataTable#getNullRowIds(int colId) method. Is it possible to use the method instead?

Copy link
Contributor Author

@xiangfu0 xiangfu0 Nov 29, 2023

Choose a reason for hiding this comment

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

No, apparently there is an issue in Pinot DataTableV4 for Pinot 0.12.1 to read null bytes value(I need to verify if this is fixed in 1.0), this exception is captured by the test testNullBehavior.

getNullRowIds also returns null from the test.

If you remove the try-catch, the testNullBehavior will fail with NPE

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to file an issue in Pinot repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I'm tracking this.

Copy link
Contributor Author

@xiangfu0 xiangfu0 Nov 30, 2023

Choose a reason for hiding this comment

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

@xiangfu0 xiangfu0 requested a review from ebyhr November 29, 2023 23:20
@ebyhr
Copy link
Member

ebyhr commented Nov 29, 2023

Fixing pinot read bytes method for v4 dataTable

Please follow the commit message guideline. https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

@xiangfu0 xiangfu0 force-pushed the fixing-pinot-bytes-read branch from 5006cc3 to 8bcfe99 Compare November 29, 2023 23:38
@ebyhr ebyhr force-pushed the fixing-pinot-bytes-read branch from 8bcfe99 to 7532ebd Compare November 29, 2023 23:42
@xiangfu0 xiangfu0 force-pushed the fixing-pinot-bytes-read branch from 7532ebd to 5c789ea Compare November 29, 2023 23:46
@ebyhr ebyhr force-pushed the fixing-pinot-bytes-read branch from 5c789ea to e4f1864 Compare November 29, 2023 23:48
@xiangfu0 xiangfu0 force-pushed the fixing-pinot-bytes-read branch from e4f1864 to 501dfc2 Compare November 29, 2023 23:58
@xiangfu0 xiangfu0 requested a review from ebyhr November 30, 2023 06:22
@ebyhr ebyhr force-pushed the fixing-pinot-bytes-read branch from 501dfc2 to 569c00b Compare November 30, 2023 06:31
@ebyhr
Copy link
Member

ebyhr commented Nov 30, 2023

[Pinot Connector] Support bytes reading in PinotSegmentPageSource for Pinot DataTable V4 compatibility

The commit message is too long and please avoid "[Pinot Connector]" prefix.

@ebyhr ebyhr merged commit c3cac47 into trinodb:master Nov 30, 2023
@github-actions github-actions bot added this to the 435 milestone Nov 30, 2023
@xiangfu0 xiangfu0 deleted the fixing-pinot-bytes-read branch November 30, 2023 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants