Skip to content

Conversation

@CircArgs
Copy link
Contributor

@CircArgs CircArgs commented Jul 12, 2022

Adding EQ, LT, GT, Null, NaN - bound and unbound versions

Predicate literals validation e.g. ensure predicates that take a single literal ultimately end up as tuple of literals as .literals, make sure unary predicates have no literals.

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Thanks @CircArgs for working on this, much appreciated! I do have some suggestions below 👍🏻

@CircArgs CircArgs requested a review from samredai July 25, 2022 16:09
@CircArgs
Copy link
Contributor Author

@Fokko please have another look when you get a chance. Thanks!

return self._literals

def __str__(self) -> str:
return f"{self.__class__.__name__}({str(self.term)}{self.literals and ', '+str(self.literals)})"
Copy link
Contributor

Choose a reason for hiding this comment

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

When I test this and literals is an empty list, I get x[] (where x is the term)


def __str__(self) -> str:
return f"({self.left} and {self.right})"
return f"And({str(self.left)}, {str(self.right)})"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is correct. This is basically just repr output. I think this should match what Java produces, which is a readable infix string.

raise AttributeError("Null is a unary predicate and takes no Literals.")

def bind(self, schema: Schema, case_sensitive: bool) -> BoundIsNull[T]:
bound_ref = self.term.bind(schema, case_sensitive)
Copy link
Contributor

Choose a reason for hiding this comment

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

A better name would be bound_term since this is not necessarily a ref.

return BoundNotEq(self.term, *self.literals)


class Eq(UnboundPredicate[T]):
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are using expression classes directly, rather than using factory methods, I think that these classes should use full names, like Equals and LessThan (or BoundEqual and BoundLessThan).

@rdblue rdblue merged commit 5e25f2b into apache:master Jul 25, 2022
@rdblue
Copy link
Contributor

rdblue commented Jul 25, 2022

Thanks, @CircArgs! I think there are some changes that would help readability, but those are minor. I merged this so that other work can continue in parallel.

@CircArgs
Copy link
Contributor Author

Thanks, @CircArgs! I think there are some changes that would help readability, but those are minor. I merged this so that other work can continue in parallel.

Thanks @rdblue. I'll open another PR soon with the changes hopefully

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants