Skip to content

feat: scaffolding work for expression#177

Merged
zeroshade merged 2 commits into
apache:mainfrom
wgtmac:expression
Oct 9, 2025
Merged

feat: scaffolding work for expression#177
zeroshade merged 2 commits into
apache:mainfrom
wgtmac:expression

Conversation

@wgtmac

@wgtmac wgtmac commented Aug 15, 2025

Copy link
Copy Markdown
Member
  • Add interface for term, unbound, bound, reference, predicate, etc.
  • Add factory to create expressions.

@wgtmac wgtmac force-pushed the expression branch 2 times, most recently from 0bd5939 to ca00452 Compare August 15, 2025 07:56
Comment thread src/iceberg/expression/term.h Outdated
Comment thread src/iceberg/expression/term.h Outdated
@wgtmac wgtmac force-pushed the expression branch 2 times, most recently from c1c25e7 to b86dda1 Compare August 17, 2025 16:08
Comment thread src/iceberg/expression/predicate.h
Comment thread src/iceberg/expression/expressions.cc Outdated
@wgtmac wgtmac force-pushed the expression branch 6 times, most recently from 17bd1f8 to 5a49c08 Compare August 19, 2025 04:59
@wgtmac

wgtmac commented Aug 19, 2025

Copy link
Copy Markdown
Member Author

This PR is ready for review. It looks pretty similar to the Java equivalent. Let me know what you think. @dongxiao1198 @lidavidm @mapleFU @yingcai-cy @zhjwpku

Comment thread src/iceberg/expression/expressions.h
Comment thread src/iceberg/expression/term.h
Comment thread src/iceberg/expression/term.h Outdated
Comment thread src/iceberg/expression/predicate.h Outdated
Comment thread src/iceberg/expression/predicate.h Outdated
@wgtmac wgtmac requested a review from mapleFU August 23, 2025 14:38
Comment thread src/iceberg/expression/expression.cc Outdated
Comment thread src/iceberg/expression/expression.h Outdated
@wgtmac wgtmac requested a review from mapleFU August 23, 2025 15:08
Comment thread src/iceberg/expression/expressions.cc
Comment thread src/iceberg/expression/predicate.h
Comment thread src/iceberg/expression/expression.cc
Comment thread src/iceberg/expression/predicate.cc
Comment thread src/iceberg/expression/predicate.cc
Comment thread src/iceberg/expression/predicate.cc Outdated
Comment thread src/iceberg/expression/predicate.cc
Comment thread src/iceberg/expression/predicate.cc
Comment thread src/iceberg/expression/term.cc
@wgtmac wgtmac mentioned this pull request Sep 3, 2025
45 tasks
@wgtmac

wgtmac commented Sep 12, 2025

Copy link
Copy Markdown
Member Author

@lidavidm Do you have any comment about this PR?

Comment thread src/iceberg/expression/expression.cc
Comment on lines +176 to +178
case Expression::Operation::kTrue:
case Expression::Operation::kFalse:
case Expression::Operation::kNot:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

shouldn't there be negations for these?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Comment thread src/iceberg/expression/expressions.cc Outdated
Comment thread src/iceberg/expression/expressions.cc Outdated
Comment thread src/iceberg/expression/expressions.cc
Comment thread src/iceberg/expression/predicate.cc
Comment thread src/iceberg/expression/predicate.cc
Comment thread src/iceberg/expression/term.cc
Comment thread src/iceberg/expression/term.cc
Comment thread src/iceberg/expression/term.cc Outdated
@wgtmac

wgtmac commented Sep 24, 2025

Copy link
Copy Markdown
Member Author

@zeroshade Thanks for your review! I think I've addressed all your comments. Please check.

Comment thread src/iceberg/expression/expression.cc Outdated
Comment on lines +182 to +183
case Expression::Operation::kAnd:
case Expression::Operation::kOr:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is interesting, we can negate and and or through the Morgan's law

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done!

@Fokko Fokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks like a great start! Thanks for working on this @wgtmac, and thanks @zeroshade, @dongxiao1198, @zhjwpku, @mapleFU, @gty404 for the reviews 🙌

@Fokko Fokko requested a review from zeroshade October 7, 2025 15:26

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good! Thanks!

@zeroshade zeroshade merged commit 622a934 into apache:main Oct 9, 2025
7 checks passed
@wgtmac

wgtmac commented Oct 9, 2025

Copy link
Copy Markdown
Member Author

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants