Skip to content

Conversation

@suibianwanwank
Copy link

Made a few tweaks to the transforms commented on PR.

@github-actions github-actions bot added the core label Mar 24, 2025

// TODO: handle nulls first/last?
filters.push(comparison);
let predicate = if threshold.sort_options.nulls_first {
Copy link
Author

Choose a reason for hiding this comment

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

Also, I have some questions about this logic. Does it handle ordering by multiple column correctly?

Copy link
Member

Choose a reason for hiding this comment

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

Worth adding a test! I think we'll want a black box test that checks that results with and without this feature are the same.

Copy link
Author

@suibianwanwank suibianwanwank left a comment

Choose a reason for hiding this comment

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

Hi, @adriangb Sorry for the delay, things got a bit busy on my end. If you have time, feel free to check out this PR~

}

#[tokio::test]
async fn test_topk_predicate_pushdown_nulls_fist() {
Copy link
Author

Choose a reason for hiding this comment

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

Two tests have been added here. Perhaps we could merge some of the code.

@adriangb adriangb merged commit be4085b into pydantic:topk-filter Mar 24, 2025
1 check passed
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.

2 participants