Skip to content
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

Redesign of the DatePartitionedQueryPlanner (formerly know as the FederatedQueryPlanner) #2717

Open
wants to merge 20 commits into
base: integration
Choose a base branch
from

Conversation

ivakegg
Copy link
Collaborator

@ivakegg ivakegg commented Jan 29, 2025

No description provided.

@ivakegg ivakegg marked this pull request as draft January 29, 2025 22:31
@ivakegg ivakegg marked this pull request as ready for review January 30, 2025 19:01
This reverts commit 549130b.
@hlgp
Copy link
Collaborator

hlgp commented Jan 31, 2025

We should probably adjust the marker concept to be one day earlier than the last old index entry timestamp. Since we usually deploy during the day, any deployment where we ceased indexing something would only be a partial day. I replicated this with a test case and any data from the last indexed date that did not get indexed (i.e. post hypothetical deployment) would be missed.

@hlgp
Copy link
Collaborator

hlgp commented Jan 31, 2025

In other tests, I'm getting empty ranges. We're still checking the fi for the field with the Hole marker, so the AndIterator appears to be defeating. Haven't fully stepped through, though, but we need more end-to-end tests up in here (up in here).


@Override
public Object visit(ASTEQNode node, Object data) {
if (isIndexed(node) && missingIndexRange(node)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure that this logic will handle queries with arithmetic or methods very well. The field or the value may be null.
See the FunctionalSetTest for examples

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but those methods will not be used in the index lookup so do they matter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I think, I see what you mean.... I will reevaluate what I am missing.

@ivakegg
Copy link
Collaborator Author

ivakegg commented Feb 3, 2025

In other tests, I'm getting empty ranges. We're still checking the fi for the field with the Hole marker, so the AndIterator appears to be defeating. Haven't fully stepped through, though, but we need more end-to-end tests up in here (up in here).

I changed this to use evaluation only instead of index value hole markers in the query to avoid the field index lookups for unindexed fields.

/**
* Visitor meant to 'push down' predicates for expressions that are not executable against the global OR field index.
*/
public class PushdownUnindexedFieldsVisitor extends RebuildingVisitor {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will you be adding any tests for this visitor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TBD

@lbschanno lbschanno linked an issue Feb 7, 2025 that may be closed by this pull request
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.

FieldIndexHoles not being used by DefaultQueryPlanner
4 participants