Accessing vortex_expr::expr::Expression children shouldn't involve explicit indices

[danking]

Currently to get the children of Between you need to know the mapping from indices to children:

    } else if let Some(opts) = expr.as_opt::<Between>() {
        let array = expr.child(0);
        let lower = expr.child(1);
        let upper = expr.child(2);
    }

Having named methods is discoverable (via auto-complete) and avoids a footgun (e.g. is the array child first or last?). We could even say the ordering of individual children is not public and you must access them via their names. That lets us reorder, though I have no reason to think we want to do that.

Concretely:

pub struct Between;

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct BetweenOptions {
    pub lower_strict: StrictComparison,
    pub upper_strict: StrictComparison,
}

pub struct BetweenChildren<'a> {
    children: &'a [Expression]
}

impl BetweenChildren<'a> {
    pub fn array() -> &'a Expression {
        return &self.children[0];
    }
    pub fn lower() -> &'a Expression {
        return &self.children[1];
    }
    pub fn upper() -> &'a Expression {
        return &self.children[2];
    }
}

impl<'a> From<&'a [Expression]> for BetweenChildren<'a> {
    fn from(value: &'a [Expression]) -> Self {
        Self { children: value }
    }
}

impl VTable for Between {
    type Options = BetweenOptions;
    type Children = BetweenChildren;
    // ...
}

pub trait VTable: 'static + Sized + Send + Sync {
    type Options: 'static + Send + Sync + Clone + Debug + Display + PartialEq + Eq + Hash;
    type Children<'a>: From<&'a [Expression]>;
    // ...
}

impl Expression {
    pub fn as_opt_with_children<'a, V: VTable>(&'a self) -> Option<(&V::Options, V::Children<'a>)> {
        self.as_opt::<V>().map(|opts| {
            (opts, V::Children::from(self.children))
        })
    }
    // ...
}

And we can use it like:

    fn stat_falsification<'a>(
        &'a self,
        options: &Self::Options,
        kids: Self::Children<'a>,
        catalog: &dyn StatsCatalog,
    ) -> Option<Expression> {
        let lhs = Binary.new_expr(
            options.lower_strict.to_operator().into(),
            [kids.lower().clone(), kids.array().clone()],
        );
        let rhs = Binary.new_expr(
            options.upper_strict.to_operator().into(),
            [kids.array().clone(), kids.upper().clone()],
        );

        Binary
            .new_expr(Operator::And, [lhs, rhs])
            .stat_falsification(catalog)
    }

[joseph-isaacs]

This looks great.

The problem with taking:

options: &Self::Options,
kids: Self::Children<'a>,

If you want to return an the input expression you cannot without a clone?

[joseph-isaacs]

Not a huge blockers just noting

[danking]

Fair point. The children are an Arc’ed slice but I suppose I could pass that to the children struct. I feel like I’m always wanting a way to parameterize over ownership vs borrowing