Skip to content

GH-39843: [C++][Parquet] Parquet binary length overflow exception should contain the length of binary#39844

Merged
mapleFU merged 8 commits intoapache:mainfrom
mapleFU:parquet/adding-str-length-for-debugging
Feb 2, 2024
Merged

GH-39843: [C++][Parquet] Parquet binary length overflow exception should contain the length of binary#39844
mapleFU merged 8 commits intoapache:mainfrom
mapleFU:parquet/adding-str-length-for-debugging

Conversation

@mapleFU
Copy link
Copy Markdown
Member

@mapleFU mapleFU commented Jan 30, 2024

Rationale for this change

See #39843

It will be great to contain a string length in decoder.

What changes are included in this PR?

change the logging of encoding

Are these changes tested?

no

Are there any user-facing changes?

more specific error logging?

@mapleFU mapleFU requested a review from wgtmac as a code owner January 30, 2024 03:07
@github-actions
Copy link
Copy Markdown

⚠️ GitHub issue #39843 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 30, 2024
@mapleFU
Copy link
Copy Markdown
Member Author

mapleFU commented Jan 30, 2024

cc @pitrou @emkornfield

@mapleFU
Copy link
Copy Markdown
Member Author

mapleFU commented Feb 1, 2024

comment fixed @pitrou

Copy link
Copy Markdown
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Feel free to merge if CI is ok :-)

@mapleFU
Copy link
Copy Markdown
Member Author

mapleFU commented Feb 1, 2024

Don't know why CI continue fails. Rebase and run ci again

@mapleFU
Copy link
Copy Markdown
Member Author

mapleFU commented Feb 2, 2024

I've checked the CI, seems these test failed for a few days. I'll merge it firstly

@mapleFU mapleFU merged commit 32bd01f into apache:main Feb 2, 2024
@mapleFU mapleFU removed the awaiting committer review Awaiting committer review label Feb 2, 2024
@conbench-apache-arrow
Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 32bd01f.

There were 2 benchmark results with an error:

There was 1 benchmark result indicating a performance regression:

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

@mapleFU
Copy link
Copy Markdown
Member Author

mapleFU commented Feb 3, 2024

Why do conbench has error =_=

@mapleFU mapleFU deleted the parquet/adding-str-length-for-debugging branch February 7, 2024 05:00
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…on should contain the length of binary (apache#39844)

### Rationale for this change

See apache#39843

It will be great to contain a string length in decoder.

### What changes are included in this PR?

change the logging of encoding

### Are these changes tested?

no

### Are there any user-facing changes?

more specific error logging?

* Closes: apache#39843

Authored-by: mwish <maplewish117@gmail.com>
Signed-off-by: mwish <maplewish117@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++][Parquet] Parquet binary length overflow exception should contain the length of binary

3 participants