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.

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

GH-38712: [Python] Remove dead code in _reconstruct_block #38714

Conversation

AlenkaF
Copy link
Member

@AlenkaF AlenkaF commented Nov 14, 2023

Rationale for this change

It seems the object case in _reconstruct_block is a dead code and is not needed anymore so therefore could be removed.

What changes are included in this PR?

Removal of the object case in _reconstruct_block code. Was also looking at the arrow_to_pandas.cc code to see if there is any dead code present and I couldn't find any.

Are these changes tested?

The change in this PR should not make any of the existing tests fail.

Are there any user-facing changes?

There shouldn't be.

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Nov 14, 2023
@jorisvandenbossche
Copy link
Member

I don't expect there to be any code in pandas_to_arrow.cc for this, I think the serialization used a different code path (and only shared this reconstruct_blocks)

@AlenkaF AlenkaF marked this pull request as ready for review November 14, 2023 11:17
@pitrou
Copy link
Member

pitrou commented Nov 14, 2023

@AlenkaF Can you fix the lint issue (see CI)?

@jorisvandenbossche
Copy link
Member

I think the linting is something R related:

> lintr::lint_package('/arrow/r')
Warning message:
> 
> 
In validate_config_file(config, config_file, default_settings) :
  Found unused settings in config '/arrow/r/.lintr': license
INFO:archery:Running Docker linter
1
Error: `docker-compose --file /home/runner/work/arrow/arrow/docker-compose.yml run --rm -e GITHUB_ACTIONS=true ubuntu-lint` exited with a non-zero exit code 1, see the process log above.

(I think I have seen it happening last week as well, although it doesn't show up on the main branch)

@pitrou
Copy link
Member

pitrou commented Nov 14, 2023

There is a Python lint failure as well, though.

@pitrou
Copy link
Member

pitrou commented Nov 14, 2023

cc @paleolimbot for the R lint issue.

@jorisvandenbossche
Copy link
Member

R lint issue might be related to #38639, although that is already merged in the branch that is being used for this PR ..

@jorisvandenbossche
Copy link
Member

There is a Python lint failure as well, though.

Ah, indeed .. The output formatting is not great, easy to look over ;)

@AlenkaF
Copy link
Member Author

AlenkaF commented Nov 14, 2023

Oh, I totally missed that as I also thought is was only R connected failure! =) Will correct 👍

@paleolimbot
Copy link
Member

It looks like you found it, but I was going to say that the "warning" appears on master as well so I don't think it's causing the job to fail: https://github.com/apache/arrow/actions/runs/6866609338/job/18673200564#step:5:981

@jorisvandenbossche jorisvandenbossche merged commit 5b17b84 into apache:main Nov 15, 2023
11 checks passed
@jorisvandenbossche jorisvandenbossche removed the awaiting merge Awaiting merge label Nov 15, 2023
@AlenkaF AlenkaF deleted the remove-old-custom-de-serialization-pandas-compat branch November 15, 2023 08:22
@jorisvandenbossche
Copy link
Member

Indeed, it was only printing the warning. But because the linter was failing because of a python lint issue further up, it was easy to think the R issue was causing the failure .. Would it be easy to avoid the warning?

Copy link

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

There were no benchmark performance regressions. 🎉

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

dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…he#38714)

### Rationale for this change

It seems the object case in `_reconstruct_block` is a dead code and is not needed anymore so therefore could be removed.

### What changes are included in this PR?

Removal of the object case in `_reconstruct_block` code. Was also looking at the `arrow_to_pandas.cc` code to see if there is any dead code present and I couldn't find any. 

### Are these changes tested?

The change in this PR should not make any of the existing tests fail.

### Are there any user-facing changes?

There shouldn't be.
* Closes: apache#38712

Authored-by: AlenkaF <frim.alenka@gmail.com>
Signed-off-by: Joris Van den Bossche <jorisvandenbossche@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.

[Python] Remove dead code in _reconstruct_block (pandas_compat.py)
5 participants