airbnb / chronon

Chronon is a data platform for serving for AI/ML applications.
Apache License 2.0
717 stars 44 forks source link

Fix bug in CatalystUtil causing `where`s to not be applied #739

Closed caiocamatta-stripe closed 6 months ago

caiocamatta-stripe commented 6 months ago

Summary

Fixes an issue in CatalystUtil that causes the where clauses in some GroupBys to not be applied.

Some notes from @piyushn-stripe's investigation: When certain selects exist in a GroupBy (such as the CAST(get_json_object(... in the unit test we added to this PR), we were hitting the ProjectExec(projectList, childPlan) branch of CatalystUtil initialize code instead of case ProjectExec(projectList, fp@FilterExec(condition, child)). This caused the wheres to not be applied.

To fix this, we added code from the WholeStageCodecgenExec branch into the ProjectExec(projectList, childPlan) branch.

Why / Goal

We observed that our Flink app (Chronon on Flink) was not filtering out events and the where clause in some GroupBys wasn't being applied at all. We narrowed it down to an issue in CatalystUtil and were able to reproduce it with a unit test.

Test Plan

We temporarily modified the code here to throw an error in the new case whc @ WholeStageCodegenExec(fp @ FilterExec(condition, child)) => case, and verified that only the GroupBys that weren't filtering correctly are affected by this change. We have hundreds of GroupBys defined, so this gives us confidence that, at least on our side, this is a safe change.

Checklist

Reviewers