Skip to content

Conversation

@a10y
Copy link
Contributor

@a10y a10y commented Jan 6, 2025

For binary expressions, we now have eq, not_eq, lt, lt_eq, gt, gt_eq, and, or (see vortex-expr/src/binary.rs). Add example usage doctests for all

For literals we have lit

For Identity we have ident

For binary expressions, we now have eq, not_eq, lt, lt_eq, gt, gt_eq, and, or

For literals we have lit

For Identity we have ident
Comment on lines +95 to +96
/// result.into_bool().unwrap().boolean_buffer(),
/// BoolArray::from_iter(vec![false, false, true]).boolean_buffer(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing I noticed while writing these doctests is that we don't have a nicer generic way of building a BooleanBuffer. You need to build a full BoolArray and then access its arrow booleanbuffer.

I added a macro so you could do something like bools![true, false, true] but that still forces you to bring in an arrow_buffer dep. We should probably use and return our own booleanbuffer type to make this more ergonomic

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we need to make our own BooleanBuffer type... relatedly, #1774

Copy link
Contributor

Choose a reason for hiding this comment

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

You can construct BooleanBuffer directly from Iter<bool>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea but then you need to add a dep on arrow_buffer where there wasn't previously, and sadly rust doesn't let you enable a dep only for doctests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh actually I'm wrong, apparently doc tests use dev-dependencies

@a10y
Copy link
Contributor Author

a10y commented Jan 6, 2025

An alternative that is a bit closer to what Arrow does is to implement the binary operators as method chains.

@robert3005
Copy link
Contributor

I think we should do this instead of chaining. Chaining is nice but not sure correct if our expressions are not an enum

@a10y a10y merged commit b0c0b95 into develop Jan 6, 2025
17 checks passed
@a10y a10y deleted the aduffy/expr-builders branch January 6, 2025 21:40
@gatesn
Copy link
Contributor

gatesn commented Jan 8, 2025

I don't know what you mean @robert3005 ? Can we do chaining too? It's better for discoverability.

@robert3005
Copy link
Contributor

We can do chaining but it's unclear how chaining works in general for arbitrarily extensible traits. Datafusion does chaining for Expressions not PhysicalExpression

@gatesn
Copy link
Contributor

gatesn commented Jan 8, 2025

Can add(lhs: ExprRef, rhs: ExprRef) be:

pub trait ExprExt: Expr {
   add(self: Arc<Self>, rhs: ExprRef) -> ExprRef {
     ..
   }
}

impl<E: Expr> ExprExt for E {}

@robert3005
Copy link
Contributor

Yes, you can do this. You still need to have many traits like that since not all expression will be defined in one place.

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.

5 participants