Skip to content

API-9331 evaluator rewrite to handle more complex expressions #77

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 7, 2025

Conversation

wlmcewen
Copy link
Contributor

@wlmcewen wlmcewen commented Oct 6, 2023

This reproduces a bug reported multiple times over the years in sparkql evaluation. We held off on a fix as it was clear the implementation needed a rewrite, and would be trivial in the v2 AST implementation we'd like to go in.

Well, that v2 branch is getting stale, and this bug has popped up again on CUR-29245, so I opted to split the difference in a rewrite that's closer to how a tree solution would operate.

Processing the expressions in the same block group is fairly trivial in the old implementation. The nested expressions is where things get tricky in the old implementation. Therefore, this implementation breaks up and groups a filter into expressions of the same block group.

From there, I process each block group starting with the most deeply nested groups. As we go back up the nesting levels, the block group is reduced to a result and a conjunction, which slides in like a placeholder for and expression. Thus the block group becomes a faux expression in the block level above it.

An AST approach would still be superior here, but the performance differences should be negligible.

Example:

# Original filter:
BlockGroup1A Eq true And BlockGroup1B Eq True And (BlockGroup2X Eq false Or BlockGroup2Y Eq false)

# block group one:
BlockGroup1A Eq true And BlockGroup1B Eq True

# block group two:
BlockGroup2X Eq false Or BlockGroup2Y Eq false

# First iteration:
BlockGroup1A Eq true And BlockGroup1B Eq True And (true)

# second iteration
true

# Finalize
true

Final thoughts: we got away with an ugly implementation for a long time, as the typical saved searches were fairly basic. As our UI and such started supporting more complex queries, we need to keep up.

@wlmcewen wlmcewen requested a review from codygustafson August 6, 2025 19:47
assert sample("Test Eq true Not Test Eq false")
assert !sample("Test Eq true Not Test Eq true")
assert sample("Test Eq true Not (Test Eq false Or Test Eq false)")
assert sample("Test Eq true Not (Test Eq false And Test Eq false)")
assert !sample("Test Eq true Not (Test Eq false Or Test Eq true)")
assert !sample("Test Eq true Not (Test Eq true Or Test Eq false)")
assert !sample("Test Eq true Not (Not Test Eq false)")
assert !sample("Test Eq false And Test Eq true Not Test Eq false")
assert !sample("Test Eq true Not (Test Eq false Or Test Eq false) And (Test Eq false Or Test Eq false)")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the primary test we've added here.

@wlmcewen wlmcewen changed the title API-9331 test case API-9331 evaluator rewrite to handle more complex expressions Aug 7, 2025
@wlmcewen wlmcewen merged commit 368eeec into sparkapi:master Aug 7, 2025
2 checks passed
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.

2 participants