vercel[bot]
commented
1 week ago The latest updates on your projects. Learn more about Vercel for Git ↗︎
Open sumeetgajjar opened 1 week ago
vercel[bot]
commented
1 week ago The latest updates on your projects. Learn more about Vercel for Git ↗︎
sumeetgajjar
commented
1 week ago Hi @KSDaemon, @RusovDmitriy, @paveltiunov - can you please review this PR?
Thanks in advance.
KSDaemon
commented
1 week ago Hi @sumeetgajjar Thank you for contributing! I'll have a look!
sumeetgajjar
commented
1 week ago 3 tests under postgres/pre-aggregations.test.js are failing.
This is primarily because the visitors cube during the pre-aggregate generation uses the following SQL:
select * from visitors WHERE ((created_at >= undefined AND created_at <= undefined))\n' +
' AND (1 = 1)\n' +Earlier (before this change) SQL was
select * from visitors WHERE ((1 = 1))\n' +
' AND (1 = 1)\n' +cube(`visitors`, {
sql: `
select * from visitors WHERE ${FILTER_PARAMS.visitors.createdAt.filter('created_at')}
AND ${FILTER_PARAMS.ReferenceOriginalSql.createdAt.filter('created_at')}
`,This makes sense as the time dimension filter values ~are~ may be unavailable during pre-aggregate generation.
sumeetgajjar
commented
1 week ago 3 tests under
postgres/pre-aggregations.test.jsare failing.This is primarily because the
visitorscube during the pre-aggregate generation uses the following SQL:select * from visitors WHERE ((created_at >= undefined AND created_at <= undefined))\n' + ' AND (1 = 1)\n' +Earlier (before this change) SQL was
select * from visitors WHERE ((1 = 1))\n' + ' AND (1 = 1)\n' +cube(`visitors`, { sql: ` select * from visitors WHERE ${FILTER_PARAMS.visitors.createdAt.filter('created_at')} AND ${FILTER_PARAMS.ReferenceOriginalSql.createdAt.filter('created_at')} `,This makes sense as the time dimension filter values ~are~ may be unavailable during pre-aggregate generation.
Given each of the DATE_OPERATORS_Where methods (i.e. inDateRangeWhere, notInDateRangeWhere, etc) exactly knows its required parameters i.e.
inDateRangeWhere both from and to are requiredbeforeDateWhere only before is requiredOne fix could be modifying each DATE_OPERATORS_Where method to return 1 = 1 when any required parameters are undefined.
@KSDaemon - Does the above proposal sound good to you?
sumeetgajjar
commented
1 week ago Hi @sumeetgajjar Thank you for contributing! I'll have a look!
Hi @KSDaemon - gentle ping, can you please take a look?
This bug is directly impacting our production workflows and is leading to incorrect data being shown to the users.
KSDaemon
commented
1 week ago Does the above proposal sound good to you?
That makes sense.
Seems to be good. I relaunched tests.
sumeetgajjar
commented
1 week ago 👍🏻 LGTM!
Thanks for the review @KSDaemon.
A couple of questions,
sumeetgajjar
commented
6 days ago Hi @KSDaemon, @paveltiunov, @igorlukanin - can you please help us merge this PR given it is already approved 😄?
The presence of the following filters in a cube query:
set,notSet,eq NULLorNOT eq NULLrenders the corresponding FILTER_PARAMS as1 = 1irrespective of the cube or filter.This is because the
renderFilterParamsmethod skips invokingfilter.conditionSqlwhen converting afilterto its corresponding SQL due to an emptyfilterParamsarray.For
setandnotSet, the filter cannot have value(s). Foreq NULLorNOT eq NULLthenullvalue is filtered by: https://github.com/cube-js/cube/blob/349597a4a1b10f6e76ef3bf18c37986cdd0962f8/packages/cubejs-schema-compiler/src/adapter/BaseFilter.ts#L115This leads to an empty
filterParamsarray, thus, skipping the filter SQL, thereby returning1 = 1.The issue becomes more serious when the query contains multiple filters on the same dimension. Consider the following example:
Cube
Query
In the above case, the rendered query would be of the form:
The right branch of the OR predicate will always evaluate TRUE, thus, the majority of the SQL optimizers will remove the LEFT branch altogether to prevent unnecessary evaluations, thereby defeating the purpose of the FILTER_PARAMS filter pushdown.
This PR aims to fix the bug by restructuring the nested if-else block, thereby also improving the readability of the same.
Check List