Skip to content

feat: support strict projection#946

Merged
liurenjie1024 merged 5 commits intoapache:mainfrom
ZENOTME:strict_projection
Apr 15, 2025
Merged

feat: support strict projection#946
liurenjie1024 merged 5 commits intoapache:mainfrom
ZENOTME:strict_projection

Conversation

@ZENOTME
Copy link
Contributor

@ZENOTME ZENOTME commented Feb 6, 2025

@ZENOTME
Copy link
Contributor Author

ZENOTME commented Feb 6, 2025

cc @Fokko @liurenjie1024 @Xuanwo @sdd

liurenjie1024
liurenjie1024 previously approved these changes Apr 14, 2025
Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Thanks @ZENOTME for this great pr, and all the tests! LGTM!

}

fn not(&mut self, _inner: Self::T) -> crate::Result<Self::T> {
unreachable!("InclusiveProjection should not be performed against Predicates that contain a Not operator. Ensure that \"Rewrite Not\" gets applied to the originating Predicate before binding it.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
unreachable!("InclusiveProjection should not be performed against Predicates that contain a Not operator. Ensure that \"Rewrite Not\" gets applied to the originating Predicate before binding it.")
unreachable!("StrictProjection should not be performed against Predicates that contain a Not operator. Ensure that \"Rewrite Not\" gets applied to the originating Predicate before binding it.")

nit: I prefer to throw Unexpected error here.

Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Thanks @ZENOTME for this pr!

@liurenjie1024 liurenjie1024 merged commit 5bdb937 into apache:main Apr 15, 2025
17 checks passed
@Fokko
Copy link
Contributor

Fokko commented Apr 15, 2025

Thanks for working on this @ZENOTME This is great!

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