Skip to content

[fix](parquet) fix dict page offset zero incompatible behaviour - #41506

Merged
morningman merged 3 commits into
apache:masterfrom
suxiaogang223:dict_page_offset_zero
Oct 10, 2024
Merged

[fix](parquet) fix dict page offset zero incompatible behaviour#41506
morningman merged 3 commits into
apache:masterfrom
suxiaogang223:dict_page_offset_zero

Conversation

@suxiaogang223

@suxiaogang223suxiaogang223 commented Sep 30, 2024

Copy link
Copy Markdown
Member

Proposed changes

Reason: https://issues.apache.org/jira/browse/ARROW-5322
Java readers(parquet-mr) handles "dictionaryPageOffset = 0" to determine if dictionary page exists where as the C readers uses "has_dictionaryPageOffset" (_isset bit in thrift message) to determine the same resulting in incompatible behaviours.
Therefore, we should consider that dicttionary page exists when both __isset.dictionary_page_offset is true and dictionary_page_offset is greater than 0.

@doris-robot

doris-robot commented Sep 30, 2024

Copy link
Copy Markdown

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR

Since 2024-03-18, the Document has been moved to doris-website.
See Doris Document.

@suxiaogang223

suxiaogang223 commented Sep 30, 2024

Copy link
Copy Markdown
MemberAuthor

run buildall

@doris-robot

doris-robot commented Sep 30, 2024

Copy link
Copy Markdown

TeamCity be ut coverage result:
Function Coverage: 37.32% (9630/25805)
Line Coverage: 28.69% (79853/278283)
Region Coverage: 28.14% (41294/146757)
Branch Coverage: 24.75% (21036/84980)
Coverage Report: http://coverage.selectdb-in.cc/coverage/6797a8f28b8ba8c9141f2672df3d9c0382ea825a_6797a8f28b8ba8c9141f2672df3d9c0382ea825a/report/index.html

@suxiaogang223

suxiaogang223 commented Oct 1, 2024

Copy link
Copy Markdown
MemberAuthor

@CodiumAI-Agent /review

@suxiaogang223

suxiaogang223 commented Oct 6, 2024

Copy link
Copy Markdown
MemberAuthor

run buildall

@QodoAI-Agent

QodoAI-Agent commented Oct 6, 2024•
edited
Loading

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 215f451)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Logic Change
The logic for determining the start offset has been modified to include a check for dictionary_page_offset > 0. Ensure this change aligns with expected behavior across all scenarios.

Logic Change
Similar to vparquet_column_chunk_reader.cpp, the logic for determining chunk_start has been updated. This needs thorough testing to confirm no regression in other use cases.

@doris-robot

doris-robot commented Oct 6, 2024

Copy link
Copy Markdown

TeamCity be ut coverage result:
Function Coverage: 37.32% (9630/25805)
Line Coverage: 28.71% (79891/278284)
Region Coverage: 28.14% (41295/146763)
Branch Coverage: 24.76% (21043/84984)
Coverage Report: http://coverage.selectdb-in.cc/coverage/215f451c9a3cf5c1d2c3c900a5f19f9d000b83ff_215f451c9a3cf5c1d2c3c900a5f19f9d000b83ff/report/index.html

@QodoAI-Agent

QodoAI-Agent commented Oct 6, 2024

Copy link
Copy Markdown

Persistent review updated to latest commit 215f451

morningman
morningman reviewed Oct 7, 2024
View reviewed changes
Comment threadbe/src/vec/exec/format/parquet/vparquet_column_chunk_reader.cpp
@suxiaogang223

suxiaogang223 commented Oct 8, 2024

Copy link
Copy Markdown
MemberAuthor

run buildall

@doris-robot

doris-robot commented Oct 8, 2024

Copy link
Copy Markdown

TeamCity be ut coverage result:
Function Coverage: 37.28% (9631/25833)
Line Coverage: 28.66% (79847/278579)
Region Coverage: 28.10% (41294/146932)
Branch Coverage: 24.73% (21040/85090)
Coverage Report: http://coverage.selectdb-in.cc/coverage/a1b18b1995510b594024c13f1c55ec2805aeb499_a1b18b1995510b594024c13f1c55ec2805aeb499/report/index.html

morningman
morningman approved these changes Oct 8, 2024
View reviewed changes

@morningmanmorningman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@github-actions

github-actionsBot commented Oct 8, 2024

Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@github-actionsgithub-actionsBot added approved Indicates a PR has been approved by one committer. reviewed labels Oct 8, 2024
@github-actions

github-actionsBot commented Oct 8, 2024

Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

@morningman
morningman merged commit 0d2550f into apache:masterOct 10, 2024
@suxiaogang223
suxiaogang223 deleted the dict_page_offset_zero branch October 10, 2024 08:02
eldenmoon pushed a commit to eldenmoon/incubator-doris that referenced this pull request Oct 10, 2024
@suxiaogang223@eldenmoon
[fix](parquet) fix dict page offset zero incompatible behaviour (apac…
d3c5750
…he#41506)
## Proposed changes
Reason: https://issues.apache.org/jira/browse/ARROW-5322
Java readers(parquet-mr) handles "dictionaryPageOffset = 0" to determine
if dictionary page exists where as the C readers uses
"has_dictionaryPageOffset" (_isset bit in thrift message) to determine
the same resulting in incompatible behaviours.
Therefore, we should consider that dicttionary page exists when both
`__isset.dictionary_page_offset` is true and `dictionary_page_offset` is
greater than 0.
cjj2010 pushed a commit to cjj2010/doris that referenced this pull request Oct 12, 2024
@suxiaogang223@cjj2010
[fix](parquet) fix dict page offset zero incompatible behaviour (apac…
5a9b2ff
…he#41506)
## Proposed changes
Reason: https://issues.apache.org/jira/browse/ARROW-5322
Java readers(parquet-mr) handles "dictionaryPageOffset = 0" to determine
if dictionary page exists where as the C readers uses
"has_dictionaryPageOffset" (_isset bit in thrift message) to determine
the same resulting in incompatible behaviours.
Therefore, we should consider that dicttionary page exists when both
`__isset.dictionary_page_offset` is true and `dictionary_page_offset` is
greater than 0.
amorynan pushed a commit to amorynan/doris that referenced this pull request Oct 12, 2024
@suxiaogang223amorynan
[fix](parquet) fix dict page offset zero incompatible behaviour (apac…
031c2ae
…he#41506)
## Proposed changes
Reason: https://issues.apache.org/jira/browse/ARROW-5322
Java readers(parquet-mr) handles "dictionaryPageOffset = 0" to determine
if dictionary page exists where as the C readers uses
"has_dictionaryPageOffset" (_isset bit in thrift message) to determine
the same resulting in incompatible behaviours.
Therefore, we should consider that dicttionary page exists when both
`__isset.dictionary_page_offset` is true and `dictionary_page_offset` is
greater than 0.
suxiaogang223 added a commit to suxiaogang223/doris that referenced this pull request Oct 16, 2024
@suxiaogang223
[fix](parquet) fix dict page offset zero incompatible behaviour (apac…
b8ab842
…he#41506)
## Proposed changes
Reason: https://issues.apache.org/jira/browse/ARROW-5322
Java readers(parquet-mr) handles "dictionaryPageOffset = 0" to determine
if dictionary page exists where as the C readers uses
"has_dictionaryPageOffset" (_isset bit in thrift message) to determine
the same resulting in incompatible behaviours.
Therefore, we should consider that dicttionary page exists when both
`__isset.dictionary_page_offset` is true and `dictionary_page_offset` is
greater than 0.
@suxiaogang223suxiaogang223 mentioned this pull request Oct 16, 2024
Merged
suxiaogang223 added a commit to suxiaogang223/doris that referenced this pull request Oct 16, 2024
@suxiaogang223
[fix](parquet) fix dict page offset zero incompatible behaviour (apac…
e30d8bd
…he#41506)
## Proposed changes
Reason: https://issues.apache.org/jira/browse/ARROW-5322
Java readers(parquet-mr) handles "dictionaryPageOffset = 0" to determine
if dictionary page exists where as the C readers uses
"has_dictionaryPageOffset" (_isset bit in thrift message) to determine
the same resulting in incompatible behaviours.
Therefore, we should consider that dicttionary page exists when both
`__isset.dictionary_page_offset` is true and `dictionary_page_offset` is
greater than 0.
@suxiaogang223suxiaogang223 mentioned this pull request Oct 16, 2024
Merged
yiguolei pushed a commit that referenced this pull request Oct 17, 2024
@suxiaogang223@morningman
[cherry-pick](branch-2.1) add parquet tvf cases and fix some parquet …
1b901f6
…bug (#41931)
## Proposed changes
pick pr:
#41683#41506#41338#39326
---------
Co-authored-by: morningman <morningman@163.com>
morningman added a commit that referenced this pull request Oct 17, 2024
@suxiaogang223@morningman
[cherry-pick](branch-3.0) fix parquet cases (#41506#41526#41683#41816
e634f0d
) (#41923)
## Proposed changes
pick prs:
#41506#41526#41683#41816
---------
Co-authored-by: morningman <morningman@163.com>
HappenLee pushed a commit to HappenLee/incubator-doris that referenced this pull request Apr 24, 2026
@suxiaogang223@HappenLee
[fix](parquet) fix dict page offset zero incompatible behaviour (apac…
ccfc72a
…he#41506)
## Proposed changes
Reason: https://issues.apache.org/jira/browse/ARROW-5322
Java readers(parquet-mr) handles "dictionaryPageOffset = 0" to determine
if dictionary page exists where as the C readers uses
"has_dictionaryPageOffset" (_isset bit in thrift message) to determine
the same resulting in incompatible behaviours.
Therefore, we should consider that dicttionary page exists when both
`__isset.dictionary_page_offset` is true and `dictionary_page_offset` is
greater than 0.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@morningmanmorningmanmorningman approved these changes
@CalvinKirsCalvinKirsCalvinKirs approved these changes

Assignees

No one assigned

Labels

approvedIndicates a PR has been approved by one committer.dev/2.1.7-mergeddev/3.0.3-mergedreviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@suxiaogang223@doris-robot@QodoAI-Agent@morningman@CalvinKirs@yiguolei