feat: implement Literal Transform#156
Conversation
| } | ||
|
|
||
| Result<std::optional<Literal>> BucketTransform::Transform(const Literal& literal) { | ||
| assert(literal.type() == source_type()); |
There was a problem hiding this comment.
Not related to this PR: we should introduce something like ARROW_DCHECK to add some explicit debug-level checks.
There was a problem hiding this comment.
Agreed, we may incorporate this alongside our logging infrastructure.
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
Thanks for bearing with me! I think we still need some changes to fully conform to the spec.
also fix some lint warnings
zhjwpku
force-pushed
the
implement_transform
branch
from
853fab3 to
4c0766a
Compare
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
+1 Thanks!
My only concern is that we need to have better test coverage of those transform utility functions. We can improve them in followup PRs as this one is getting large enough.
| using namespace std::chrono; // NOLINT | ||
| switch (source_type()->type_id()) { | ||
| case TypeId::kDate: { | ||
| auto value = std::get<int32_t>(literal.value()); |
There was a problem hiding this comment.
nit: I think we need also move all of these transform functions to a dedicated utility file and add exhaustive test cases. Could you create an issue for this?
|
@Fokko @zeroshade Could you help review this? Thanks! |
mapleFU
left a comment
mapleFU
left a comment
There was a problem hiding this comment.
Perhaps we can assert return literal type is equal to result type?
That would require introducing a temporary variable before the return statement. I think if we have thorough test cases, the assert might not be necessary. |
src/iceberg/util/truncate_utils.h
Outdated
|
|
||
| namespace iceberg { | ||
|
|
||
| ICEBERG_EXPORT class TruncateUtils { |
There was a problem hiding this comment.
| ICEBERG_EXPORT class TruncateUtils { | |
| class ICEBERG_EXPORT TruncateUtils { |
There was a problem hiding this comment.
I initially used this approach, but the pre-commit clang-format hook misformatted the file, so I followed the StringUtils approach instead.
There was a problem hiding this comment.
Ok, I found that upgrade https://github.com/pre-commit/mirrors-clang-format to v20.1.8 works, so changed to the suggested approach, thanks.
5 participants
No description provided.