Skip to content

feat: implement transform ResultType #132

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jul 25, 2025
Merged

feat: implement transform ResultType #132

merged 4 commits into from
Jul 25, 2025

Conversation

@zhjwpku
Copy link
Collaborator

@zhjwpku zhjwpku commented Jun 29, 2025

No description provided.

@wgtmac
Copy link
Member

wgtmac commented Jun 29, 2025 

  Connection #0 to host www.apache.org left intact

  Issue another request to this URL:
  'https://archive.apache.org/dist/thrift/0.20.0/thrift-0.20.0.tar.gz'

  timeout on name lookup is not supported

  Host archive.apache.org:443 was resolved.

We have seen quite a lot timeout from downloading thrift-0.20.0. Should we fix this in the Arrow repo to use more stable URL? @lidavidm @raulcd

@wgtmac
Copy link
Member

wgtmac commented Jun 29, 2025

cc @gty404

gty404
gty404 reviewed Jun 30, 2025
View reviewed changes
src/iceberg/transform_function.cc Outdated
case TypeId::kUuid:
case TypeId::kFixed:
case TypeId::kBinary:
return std::make_shared<IntType>();
Copy link
Contributor

@gty404 gty404 Jun 30, 2025

Choose a reason for hiding this comment

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

Do you need to define a static variable to avoid creating a new object every time, or add a singleton to the corresponding type? The following return values are also similar.

Copy link
Collaborator Author

@zhjwpku zhjwpku Jun 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I've always wanted to add a singleton for each primitive type. If we reach a consensus, I can create a separate PR to implement that. WDYT @wgtmac @lidavidm @gty404 @mapleFU @Fokko @raulcd

Copy link
Member

@lidavidm lidavidm Jun 30, 2025

Choose a reason for hiding this comment

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

+1 Sorry - I had also meant to but as you can see recently I don't have the time.

Copy link
Collaborator Author

@zhjwpku zhjwpku Jul 7, 2025

Choose a reason for hiding this comment

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

Changed to the singleton way, please take a look. @gty404

@raulcd
Copy link
Member

raulcd commented Jun 30, 2025

We have seen quite a lot timeout from downloading thrift-0.20.0. Should we fix this in the Arrow repo to use more stable URL? @lidavidm @raulcd

There is an issue and a WIP PR (some issues were being worked on)

@zhjwpku zhjwpku force-pushed the implement_result_type branch 2 times, most recently from 3511cdf to d56f432 Compare July 7, 2025 13:54
gty404
gty404 approved these changes Jul 7, 2025
View reviewed changes
Copy link
Contributor

@gty404 gty404 left a comment

Choose a reason for hiding this comment

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

The type objects in test can also be modified to singleton.

@zhjwpku zhjwpku force-pushed the implement_result_type branch from d56f432 to bb47fb5 Compare July 7, 2025 14:01
@zhjwpku
Copy link
Collaborator Author

zhjwpku commented Jul 7, 2025

The type objects in test can also be modified to singleton.

Yeah, that's an oversight, fixed.

@zhjwpku zhjwpku requested review from lidavidm and wgtmac July 7, 2025 14:32
wgtmac
wgtmac reviewed Jul 8, 2025
View reviewed changes
@zhjwpku zhjwpku force-pushed the implement_result_type branch from bb47fb5 to 41c92a6 Compare July 21, 2025 16:16
@zhjwpku zhjwpku requested a review from wgtmac July 22, 2025 00:19
wgtmac
wgtmac approved these changes Jul 22, 2025
View reviewed changes
src/iceberg/transform_function.cc Outdated
Result<std::shared_ptr<Type>> BucketTransform::ResultType() const {
return NotImplemented("BucketTransform::result_type");
auto src_type = source_type();
switch (src_type->type_id()) {
Copy link
Member

@wgtmac wgtmac Jul 22, 2025

Choose a reason for hiding this comment

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

Shouldn't we check this in the Make function and blindly return int32() here? We can add static bool BucketTransform::Accepts(const std::shared_ptr<Type>& source_type). I'm open to discuss this but not a blocker for this PR.

Copy link
Collaborator Author

@zhjwpku zhjwpku Jul 22, 2025

Choose a reason for hiding this comment

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

True, changed to suggested, I didn't not add Accepts though.

@zeroshade zeroshade merged commit db44f75 into apache:main Jul 25, 2025
7 checks passed
@zhjwpku zhjwpku deleted the implement_result_type branch July 26, 2025 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zeroshade zeroshade zeroshade approved these changes

@wgtmac wgtmac wgtmac approved these changes

@lidavidm lidavidm Awaiting requested review from lidavidm

+1 more reviewer

@gty404 gty404 gty404 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@zhjwpku @wgtmac @raulcd @lidavidm @zeroshade @gty404