Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-36845: [C++][Python] Allow type promotion on
pa.concat_tables
#36846GH-36845: [C++][Python] Allow type promotion on
pa.concat_tables
#36846Changes from 32 commits
dfcefbc
0b223be
7abdc13
f993f90
2dc904a
daf3392
a2547f8
34020dc
651c4cc
940cfe5
682dc98
b4a18a0
057ded1
2de01ed
f09ab8d
fde4c52
d14814f
fffb846
e4fc346
ceac692
1bb6642
6992934
9d5195d
16bdc92
cbedc8b
535efb9
ff9f14f
2451387
f6a43ff
8808f28
c37f999
1c5643c
eb45145
45f15cc
b16e849
5290949
a17c4a5
8f996f9
f1521a0
be0db91
b2b8b4c
cc00894
895f87a
cad5e1d
f1301bd
a8802c5
3a25ac7
9722b74
af62b99
78460ec
31ec8e6
a9452c4
ed58763
d749851
beacead
d35c446
c3ca1f6
94dc45f
efcee5a
81573a9
e47b41d
015b5a8
318d65e
742156a
5f570a7
0d225ac
8ade8fc
c9062b8
a1cdba7
6f93f14
4ecc909
f7c6328
ea860d9
b12cc66
42faccc
de76774
09b58d7
0fa2e1a
2a505b1
a76f7f3
59a267f
5eb6c29
977c703
b038491
8beb9a1
cfc739b
493c4e9
e19f064
61ed125
bf2842e
5dfae58
e10aed9
a216d48
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make this
TypeError
(and in the nullability test above as well, actually).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you forget to change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went a bit back and forth here, but it looks like this one slipped through the cracks. I'm okay with changing the above test, but that will change the previous behavior. (but I think we're okay here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to avoid pulling compute headers in this
.h
file.Perhaps you can forward-declare
CastOptions
and pass it by const-ref:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not seem to find the class when building the tests:
Any pointers?
Also, I think it would be nice to set the
CastOptions
to::Safe()
by default, but I'm not able to get this working with the forward reference.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the function
PromoteTableToSchema
that fails linking in your error message above. Meaning you probably didn't implement that particular signature?Yes, this is why I suggested two signatures, one with
CastOptions
and one without.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks. I changed the
CastOptions
to a pointer because I can't forward declare theCastOptions::Safe()
afaik. If there is a nullpointer, it will be set toSafe()
within the#ifdef ARROW_COMPUTE
block. Let me know what you thinkThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About including the compute headers here, @pitrou are you fine with the current state, or do you have another suggestion how to avoid it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not fine, and the suggestion I made in https://github.com/apache/arrow/pull/36846/files/1c5643c4a855ca7a29fdbc4e3b83acd4e3d2af96#r1290117358 still holds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the import 👍 Digging into the failing CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you tests for when casting fails because of overflow or other issues?
It would also showcase why the distinction between
TypeError
andInvalid
is useful.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you haven't addressed this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a test for combining a
decimal(76, 75)
and aint64