You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "liurenjie1024 (via GitHub)" <gi...@apache.org> on 2024/03/06 08:06:44 UTC
[PR] feat: Implement binding expression [iceberg-rust]
liurenjie1024 opened a new pull request, #231:
URL: https://github.com/apache/iceberg-rust/pull/231
Close #149
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#issuecomment-1990023145
> Let's say that you have a table with number: i32 column. At some point, you'll evolve this column to number: i64. New data will be written as a long, but the old data will still be stored as an integer. When you evaluate the schema of the parquet file with number = 2^32 + 1, then this cannot be contained in the integer field, and further processing of the Parquet file can be skipped.
Yes, I have noticed that it's `belowMin`/`aboveMax` literals in python. I'll implement this later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#discussion_r1520407376
##########
crates/iceberg/src/expr/predicate.rs:
##########
@@ -172,6 +223,146 @@ pub enum Predicate {
Set(SetExpression<Reference>),
}
+impl Bind for Predicate {
+ type Bound = BoundPredicate;
+
+ fn bind(self, schema: SchemaRef, case_sensitive: bool) -> Result<BoundPredicate> {
+ match self {
+ Predicate::And(expr) => {
+ let bound_expr = expr.bind(schema, case_sensitive)?;
+
+ let [left, right] = bound_expr.inputs;
+ Ok(match (left, right) {
+ (_, r) if matches!(&*r, &BoundPredicate::AlwaysFalse) => {
+ BoundPredicate::AlwaysFalse
+ }
+ (l, _) if matches!(&*l, &BoundPredicate::AlwaysFalse) => {
+ BoundPredicate::AlwaysFalse
+ }
+ (left, r) if matches!(&*r, &BoundPredicate::AlwaysTrue) => *left,
+ (l, right) if matches!(&*l, &BoundPredicate::AlwaysTrue) => *right,
+ (left, right) => BoundPredicate::And(LogicalExpression::new([left, right])),
+ })
+ }
+ Predicate::Not(expr) => {
+ let bound_expr = expr.bind(schema, case_sensitive)?;
+ let [inner] = bound_expr.inputs;
+ Ok(match inner {
+ e if matches!(&*e, &BoundPredicate::AlwaysTrue) => BoundPredicate::AlwaysFalse,
+ e if matches!(&*e, &BoundPredicate::AlwaysFalse) => BoundPredicate::AlwaysTrue,
+ e => BoundPredicate::Not(LogicalExpression::new([e])),
+ })
+ }
+ Predicate::Or(expr) => {
+ let bound_expr = expr.bind(schema, case_sensitive)?;
+ let [left, right] = bound_expr.inputs;
+ Ok(match (left, right) {
+ (_, r) if matches!(&*r, &BoundPredicate::AlwaysTrue) => {
+ BoundPredicate::AlwaysTrue
+ }
+ (l, _) if matches!(&*l, &BoundPredicate::AlwaysTrue) => {
+ BoundPredicate::AlwaysTrue
+ }
+ (left, r) if matches!(&*r, &BoundPredicate::AlwaysFalse) => *left,
+ (l, right) if matches!(&*l, &BoundPredicate::AlwaysFalse) => *right,
+ (left, right) => BoundPredicate::Or(LogicalExpression::new([left, right])),
+ })
+ }
+ Predicate::Unary(expr) => {
+ let bound_expr = expr.bind(schema, case_sensitive)?;
+
+ match &bound_expr.op {
+ &PredicateOperator::IsNull => {
+ if bound_expr.term.field().required {
+ return Ok(BoundPredicate::AlwaysFalse);
+ }
+ }
+ &PredicateOperator::NotNull => {
+ if bound_expr.term.field().required {
+ return Ok(BoundPredicate::AlwaysTrue);
+ }
+ }
+ &PredicateOperator::IsNan | &PredicateOperator::NotNan => {
+ if !bound_expr.term.field().field_type.is_floating_type() {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "Expecting floating point type, but found {}",
+ bound_expr.term.field().field_type
+ ),
+ ));
+ }
+ }
+ op => {
+ return Err(Error::new(
+ ErrorKind::Unexpected,
+ format!("Expecting unary operator,but found {op}"),
Review Comment:
```suggestion
format!("Expecting unary operator, but found {op}"),
```
##########
crates/iceberg/src/expr/predicate.rs:
##########
@@ -172,6 +223,146 @@ pub enum Predicate {
Set(SetExpression<Reference>),
}
+impl Bind for Predicate {
+ type Bound = BoundPredicate;
+
+ fn bind(self, schema: SchemaRef, case_sensitive: bool) -> Result<BoundPredicate> {
+ match self {
+ Predicate::And(expr) => {
+ let bound_expr = expr.bind(schema, case_sensitive)?;
+
+ let [left, right] = bound_expr.inputs;
+ Ok(match (left, right) {
+ (_, r) if matches!(&*r, &BoundPredicate::AlwaysFalse) => {
+ BoundPredicate::AlwaysFalse
+ }
+ (l, _) if matches!(&*l, &BoundPredicate::AlwaysFalse) => {
+ BoundPredicate::AlwaysFalse
+ }
+ (left, r) if matches!(&*r, &BoundPredicate::AlwaysTrue) => *left,
+ (l, right) if matches!(&*l, &BoundPredicate::AlwaysTrue) => *right,
+ (left, right) => BoundPredicate::And(LogicalExpression::new([left, right])),
+ })
+ }
+ Predicate::Not(expr) => {
+ let bound_expr = expr.bind(schema, case_sensitive)?;
+ let [inner] = bound_expr.inputs;
+ Ok(match inner {
+ e if matches!(&*e, &BoundPredicate::AlwaysTrue) => BoundPredicate::AlwaysFalse,
+ e if matches!(&*e, &BoundPredicate::AlwaysFalse) => BoundPredicate::AlwaysTrue,
+ e => BoundPredicate::Not(LogicalExpression::new([e])),
+ })
+ }
+ Predicate::Or(expr) => {
+ let bound_expr = expr.bind(schema, case_sensitive)?;
+ let [left, right] = bound_expr.inputs;
+ Ok(match (left, right) {
+ (_, r) if matches!(&*r, &BoundPredicate::AlwaysTrue) => {
+ BoundPredicate::AlwaysTrue
+ }
+ (l, _) if matches!(&*l, &BoundPredicate::AlwaysTrue) => {
+ BoundPredicate::AlwaysTrue
+ }
Review Comment:
Out of curiosity, why not collapse this into a single statement?
```suggestion
(l, r) if matches!(&*r, &BoundPredicate::AlwaysTrue) || matches!(&*l, &BoundPredicate::AlwaysTrue) => {
BoundPredicate::AlwaysTrue
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 merged PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#discussion_r1519131858
##########
crates/iceberg/src/expr/predicate.rs:
##########
@@ -406,13 +635,239 @@ mod tests {
#[test]
fn test_predicate_negate_set() {
- let expression = Reference::new("a").is_in(HashSet::from([Datum::long(5), Datum::long(6)]));
+ let expression = Reference::new("a").is_in([Datum::long(5), Datum::long(6)]);
- let expected =
- Reference::new("a").is_not_in(HashSet::from([Datum::long(5), Datum::long(6)]));
+ let expected = Reference::new("a").is_not_in([Datum::long(5), Datum::long(6)]);
Review Comment:
Sorry, which one? Do you mean the `negate` case or `bind` case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#discussion_r1517228198
##########
crates/iceberg/src/expr/predicate.rs:
##########
@@ -55,6 +60,29 @@ impl<T, const N: usize> LogicalExpression<T, N> {
}
}
+impl<T: Bind, const N: usize> Bind for LogicalExpression<T, N> {
+ type Bound = LogicalExpression<T::Bound, N>;
+
+ fn bind(self, schema: SchemaRef, case_sensitive: bool) -> Result<Self::Bound> {
+ let mut bound_inputs = MaybeUninit::<[Box<T::Bound>; N]>::uninit();
+ for (i, input) in self.inputs.into_iter().enumerate() {
+ let input = input.bind(schema.clone(), case_sensitive)?;
+ // SAFETY: The pointer is valid from [`MaybeUninit`].
+ unsafe {
Review Comment:
It feels like a bit of a premature optimisation to introduce `unsafe` and `MaybeUninit` here when safe code will probably do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#issuecomment-1987569345
I took a look at python's code again and realized that we should not couple not rewrite with binding, since not rewrite is not as widely used as binding, and we still need a rewriter. cc @sdd
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#issuecomment-1986848432
> Changes from review look good to me. Because we're now doing the NOT-elimination here at bind time, does this mean that this PR effectively completes #150 as well?
Seem like we made it๐
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#discussion_r1519133419
##########
crates/iceberg/src/expr/predicate.rs:
##########
@@ -406,13 +635,239 @@ mod tests {
#[test]
fn test_predicate_negate_set() {
- let expression = Reference::new("a").is_in(HashSet::from([Datum::long(5), Datum::long(6)]));
+ let expression = Reference::new("a").is_in([Datum::long(5), Datum::long(6)]);
- let expected =
- Reference::new("a").is_not_in(HashSet::from([Datum::long(5), Datum::long(6)]));
+ let expected = Reference::new("a").is_not_in([Datum::long(5), Datum::long(6)]);
Review Comment:
Ok, I got it. Predicate itself needs `Reference` inside. It's not something like `Result`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#issuecomment-1986847123
Changes from review look good to me. Because we're now doing the NOT-elimination here at bind time, does this mean that this PR effectively completes https://github.com/apache/iceberg-rust/issues/150 as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#discussion_r1517231867
##########
crates/iceberg/src/expr/predicate.rs:
##########
@@ -172,6 +228,146 @@ pub enum Predicate {
Set(SetExpression<Reference>),
}
+impl Bind for Predicate {
+ type Bound = BoundPredicate;
+
+ fn bind(self, schema: SchemaRef, case_sensitive: bool) -> Result<BoundPredicate> {
+ match self {
+ Predicate::And(expr) => {
+ let bound_expr = expr.bind(schema, case_sensitive)?;
+
+ let [left, right] = bound_expr.inputs;
+ Ok(match (left, right) {
+ (_, r) if matches!(&*r, &BoundPredicate::AlwaysFalse) => {
+ BoundPredicate::AlwaysFalse
+ }
+ (l, _) if matches!(&*l, &BoundPredicate::AlwaysFalse) => {
+ BoundPredicate::AlwaysFalse
+ }
+ (left, r) if matches!(&*r, &BoundPredicate::AlwaysTrue) => *left,
+ (l, right) if matches!(&*l, &BoundPredicate::AlwaysTrue) => *right,
+ (left, right) => BoundPredicate::And(LogicalExpression::new([left, right])),
+ })
+ }
+ Predicate::Not(expr) => {
+ let bound_expr = expr.bind(schema, case_sensitive)?;
+ let [inner] = bound_expr.inputs;
+ Ok(match inner {
+ e if matches!(&*e, &BoundPredicate::AlwaysTrue) => BoundPredicate::AlwaysFalse,
+ e if matches!(&*e, &BoundPredicate::AlwaysFalse) => BoundPredicate::AlwaysTrue,
+ e => BoundPredicate::Not(LogicalExpression::new([e])),
Review Comment:
You could use `negate()` here to do `Not` elimination :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#issuecomment-1983016798
This pr is ready for review. cc @Fokko @Xuanwo @sdd
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#discussion_r1519137105
##########
crates/iceberg/src/expr/predicate.rs:
##########
@@ -55,6 +60,24 @@ impl<T, const N: usize> LogicalExpression<T, N> {
}
}
+impl<T: Bind, const N: usize> Bind for LogicalExpression<T, N>
+where
+ T::Bound: Sized,
+{
+ type Bound = LogicalExpression<T::Bound, N>;
+
+ fn bind(self, schema: SchemaRef, case_sensitive: bool) -> Result<Self::Bound> {
+ let mut outputs: [Option<Box<T::Bound>>; N] = array_init(|_| None);
+ for (i, input) in self.inputs.into_iter().enumerate() {
+ outputs[i] = Some(Box::new(input.bind(schema.clone(), case_sensitive)?));
+ }
+
+ // It's safe to use `unwrap` here since they are all `Some`.
Review Comment:
Makes sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#issuecomment-1987797610
> I took a look at python's code again and realised that we should not couple not rewrite with binding, since not rewrite is not as widely used as binding, and we still need a rewriter. cc @sdd
Aah, ok, that's a shame as it would have been simpler to do it at bind time. Never mind! :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#discussion_r1518562233
##########
crates/iceberg/src/expr/predicate.rs:
##########
@@ -55,6 +60,29 @@ impl<T, const N: usize> LogicalExpression<T, N> {
}
}
+impl<T: Bind, const N: usize> Bind for LogicalExpression<T, N> {
+ type Bound = LogicalExpression<T::Bound, N>;
+
+ fn bind(self, schema: SchemaRef, case_sensitive: bool) -> Result<Self::Bound> {
+ let mut bound_inputs = MaybeUninit::<[Box<T::Bound>; N]>::uninit();
+ for (i, input) in self.inputs.into_iter().enumerate() {
+ let input = input.bind(schema.clone(), case_sensitive)?;
+ // SAFETY: The pointer is valid from [`MaybeUninit`].
+ unsafe {
Review Comment:
Thanks for review. I'm trying hard to avoid allocation as much as possible, and I found that the crate `array_init` is quite good at this, and no unsafe any more.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#discussion_r1519136619
##########
crates/iceberg/src/expr/predicate.rs:
##########
@@ -55,6 +60,24 @@ impl<T, const N: usize> LogicalExpression<T, N> {
}
}
+impl<T: Bind, const N: usize> Bind for LogicalExpression<T, N>
+where
+ T::Bound: Sized,
+{
+ type Bound = LogicalExpression<T::Bound, N>;
+
+ fn bind(self, schema: SchemaRef, case_sensitive: bool) -> Result<Self::Bound> {
+ let mut outputs: [Option<Box<T::Bound>>; N] = array_init(|_| None);
+ for (i, input) in self.inputs.into_iter().enumerate() {
+ outputs[i] = Some(Box::new(input.bind(schema.clone(), case_sensitive)?));
+ }
+
+ // It's safe to use `unwrap` here since they are all `Some`.
Review Comment:
My first version uses `MaybeUninit`, but it involves `unsafe` codes. I think we should avoid `unsafe` as much as possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#discussion_r1517227108
##########
crates/iceberg/src/expr/mod.rs:
##########
@@ -147,6 +149,14 @@ impl PredicateOperator {
}
}
+/// Bind expression to a schema.
+pub trait Bind {
+ /// The type of the bounded result.
Review Comment:
Small typo, should be "The type of the bound result."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#discussion_r1517243146
##########
crates/iceberg/src/expr/predicate.rs:
##########
@@ -406,13 +640,239 @@ mod tests {
#[test]
fn test_predicate_negate_set() {
- let expression = Reference::new("a").is_in(HashSet::from([Datum::long(5), Datum::long(6)]));
+ let expression = Reference::new("a").is_in([Datum::long(5), Datum::long(6)]);
- let expected =
- Reference::new("a").is_not_in(HashSet::from([Datum::long(5), Datum::long(6)]));
+ let expected = Reference::new("a").is_not_in([Datum::long(5), Datum::long(6)]);
let result = expression.negate();
assert_eq!(result, expected);
}
+
+ fn table_schema_simple() -> SchemaRef {
+ Arc::new(
+ Schema::builder()
+ .with_schema_id(1)
+ .with_identifier_field_ids(vec![2])
+ .with_fields(vec![
+ NestedField::optional(1, "foo", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::required(2, "bar", Type::Primitive(PrimitiveType::Int)).into(),
+ NestedField::optional(3, "baz", Type::Primitive(PrimitiveType::Boolean)).into(),
+ ])
+ .build()
+ .unwrap(),
+ )
+ }
+
+ #[test]
+ fn test_bind_is_null() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("foo").is_null();
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "foo IS NULL");
+ }
+
+ #[test]
+ fn test_bind_is_null_required() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("bar").is_null();
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "False");
+ }
+
+ #[test]
+ fn test_bind_is_not_null() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("foo").is_not_null();
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "foo IS NOT NULL");
+ }
+
+ #[test]
+ fn test_bind_is_not_null_required() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("bar").is_not_null();
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "True");
+ }
+
+ #[test]
+ fn test_bind_less_than() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("bar").less_than(Datum::int(10));
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "bar < 10");
+ }
+
+ #[test]
+ fn test_bind_less_than_wrong_type() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("bar").less_than(Datum::string("abcd"));
+ let bound_expr = expr.bind(schema, true);
+ assert!(bound_expr.is_err());
+ }
+
+ #[test]
+ fn test_bind_greater_than_or_eq() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("bar").greater_than_or_equal_to(Datum::int(10));
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "bar >= 10");
+ }
+
+ #[test]
+ fn test_bind_greater_than_or_eq_wrong_type() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("bar").greater_than_or_equal_to(Datum::string("abcd"));
+ let bound_expr = expr.bind(schema, true);
+ assert!(bound_expr.is_err());
+ }
+
+ #[test]
+ fn test_bind_in() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("bar").is_in([Datum::int(10), Datum::int(20)]);
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "bar IN (20, 10)");
+ }
+
+ #[test]
+ fn test_bind_in_empty() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("bar").is_in(vec![]);
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "False");
+ }
+
+ #[test]
+ fn test_bind_in_one_literal() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("bar").is_in(vec![Datum::int(10)]);
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "bar = 10");
+ }
+
+ #[test]
+ fn test_bind_in_wrong_type() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("bar").is_in(vec![Datum::int(10), Datum::string("abcd")]);
+ let bound_expr = expr.bind(schema, true);
+ assert!(bound_expr.is_err());
+ }
+
+ #[test]
+ fn test_bind_not_in() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("bar").is_not_in([Datum::int(10), Datum::int(20)]);
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "bar NOT IN (20, 10)");
+ }
+
+ #[test]
+ fn test_bind_not_in_empty() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("bar").is_not_in(vec![]);
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "True");
+ }
+
+ #[test]
+ fn test_bind_not_in_one_literal() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("bar").is_not_in(vec![Datum::int(10)]);
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "bar != 10");
+ }
+
+ #[test]
+ fn test_bind_not_in_wrong_type() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("bar").is_not_in([Datum::int(10), Datum::string("abcd")]);
+ let bound_expr = expr.bind(schema, true);
+ assert!(bound_expr.is_err());
+ }
+
+ #[test]
+ fn test_bind_and() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("bar")
+ .less_than(Datum::int(10))
+ .and(Reference::new("foo").is_null());
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "(bar < 10) AND (foo IS NULL)");
+ }
+
+ #[test]
+ fn test_bind_and_always_false() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("foo")
+ .less_than(Datum::string("abcd"))
+ .and(Reference::new("bar").is_null());
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "False");
+ }
+
+ #[test]
+ fn test_bind_and_always_true() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("foo")
+ .less_than(Datum::string("abcd"))
+ .and(Reference::new("bar").is_not_null());
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), r#"foo < "abcd""#);
+ }
+
+ #[test]
+ fn test_bind_or() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("bar")
+ .less_than(Datum::int(10))
+ .or(Reference::new("foo").is_null());
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "(bar < 10) OR (foo IS NULL)");
+ }
+
+ #[test]
+ fn test_bind_or_always_true() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("foo")
+ .less_than(Datum::string("abcd"))
+ .or(Reference::new("bar").is_not_null());
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "True");
+ }
+
+ #[test]
+ fn test_bind_or_always_false() {
+ let schema = table_schema_simple();
+ let expr = Reference::new("foo")
+ .less_than(Datum::string("abcd"))
+ .or(Reference::new("bar").is_null());
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), r#"foo < "abcd""#);
+ }
+
+ #[test]
+ fn test_bind_not() {
+ let schema = table_schema_simple();
+ let expr = !Reference::new("bar").less_than(Datum::int(10));
+ let bound_expr = expr.bind(schema, true).unwrap();
+ assert_eq!(&format!("{bound_expr}"), "NOT (bar < 10)");
Review Comment:
`bar >= 10` if you use `negate()` :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#discussion_r1519129192
##########
crates/iceberg/src/expr/predicate.rs:
##########
@@ -406,13 +635,239 @@ mod tests {
#[test]
fn test_predicate_negate_set() {
- let expression = Reference::new("a").is_in(HashSet::from([Datum::long(5), Datum::long(6)]));
+ let expression = Reference::new("a").is_in([Datum::long(5), Datum::long(6)]);
- let expected =
- Reference::new("a").is_not_in(HashSet::from([Datum::long(5), Datum::long(6)]));
+ let expected = Reference::new("a").is_not_in([Datum::long(5), Datum::long(6)]);
Review Comment:
This test case is unclear to me. Perhaps we should directly compare it with the actual result? It's possible that both `is_in` and `is_not_in` were implemented incorrectly.
##########
crates/iceberg/src/expr/predicate.rs:
##########
@@ -55,6 +60,24 @@ impl<T, const N: usize> LogicalExpression<T, N> {
}
}
+impl<T: Bind, const N: usize> Bind for LogicalExpression<T, N>
+where
+ T::Bound: Sized,
+{
+ type Bound = LogicalExpression<T::Bound, N>;
+
+ fn bind(self, schema: SchemaRef, case_sensitive: bool) -> Result<Self::Bound> {
+ let mut outputs: [Option<Box<T::Bound>>; N] = array_init(|_| None);
+ for (i, input) in self.inputs.into_iter().enumerate() {
+ outputs[i] = Some(Box::new(input.bind(schema.clone(), case_sensitive)?));
+ }
+
+ // It's safe to use `unwrap` here since they are all `Some`.
Review Comment:
A bit confused. Why we need to create `[Some(T)]` and unwrap them one by one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#discussion_r1519131277
##########
crates/iceberg/src/expr/predicate.rs:
##########
@@ -55,6 +60,24 @@ impl<T, const N: usize> LogicalExpression<T, N> {
}
}
+impl<T: Bind, const N: usize> Bind for LogicalExpression<T, N>
+where
+ T::Bound: Sized,
+{
+ type Bound = LogicalExpression<T::Bound, N>;
+
+ fn bind(self, schema: SchemaRef, case_sensitive: bool) -> Result<Self::Bound> {
+ let mut outputs: [Option<Box<T::Bound>>; N] = array_init(|_| None);
+ for (i, input) in self.inputs.into_iter().enumerate() {
+ outputs[i] = Some(Box::new(input.bind(schema.clone(), case_sensitive)?));
+ }
+
+ // It's safe to use `unwrap` here since they are all `Some`.
Review Comment:
Because without this, we can't initialize the array.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Implement binding expression [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #231:
URL: https://github.com/apache/iceberg-rust/pull/231#discussion_r1519132180
##########
crates/iceberg/src/expr/predicate.rs:
##########
@@ -55,6 +60,24 @@ impl<T, const N: usize> LogicalExpression<T, N> {
}
}
+impl<T: Bind, const N: usize> Bind for LogicalExpression<T, N>
+where
+ T::Bound: Sized,
+{
+ type Bound = LogicalExpression<T::Bound, N>;
+
+ fn bind(self, schema: SchemaRef, case_sensitive: bool) -> Result<Self::Bound> {
+ let mut outputs: [Option<Box<T::Bound>>; N] = array_init(|_| None);
+ for (i, input) in self.inputs.into_iter().enumerate() {
+ outputs[i] = Some(Box::new(input.bind(schema.clone(), case_sensitive)?));
+ }
+
+ // It's safe to use `unwrap` here since they are all `Some`.
Review Comment:
How about using `MaybeUninit`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org