-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix: compute fixes for extension types #171
base: main
Are you sure you want to change the base?
Conversation
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'm not sure what are the problems to be fixed...
It may be better that:
- We use one pull request per problem
- We add how to reproduce the problem, actual result and expected result in issue/PR description
uidStorage, _ = scalar.MakeScalarParam(exampleUUID[:], | ||
&arrow.FixedSizeBinaryType{ByteWidth: 16}) | ||
uid = scalar.NewExtensionScalar(uidStorage, extensions.NewUUIDType()) |
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.
uid
-> uuid
?
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.
uuid
is used as the name of the uuid
package, so I'd end up with a conflict / issue if I use the name for the variable here. I can rename it as something else though if we prefer.
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.
Ah! uuidStorage
and uuidScalar
may be better. (I feel that uid
is "user id"...)
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.
will do!
@@ -613,7 +632,7 @@ func executeScalarBatch(ctx context.Context, input compute.ExecBatch, exp expr.E | |||
result.Release() |
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.
Do we need result = nil
here? (Can we return Release()
-ed result
?)
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.
hmm, probably safer to make it nil
you're right. just in case.
I can separate the change in the compute side from the change on the parquet side. While the changes on the compute side are fixing 2 different issues, they are intertwined and would end up having duplicate code between them if i separate them into different PRs. I'll split the PR into those two separate ones tomorrow. Essentially it's related to just reading UUID fields from parquet files and utilizing UUID extension types in the compute package to perform comparisons, along with a small error propagation issue. |
While working on github.com/apache/iceberg-go to enable reading data, I came across a few issues when dealing with UUID / extension types and some other small compute things. This fixes those issues and adds tests to account for them.