-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix(schema-compiler): add support for time filters and rolling windows and fix subquery aliasing in oracle #10066
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
base: master
Are you sure you want to change the base?
fix(schema-compiler): add support for time filters and rolling windows and fix subquery aliasing in oracle #10066
Conversation
… for Oracle compatibility Oracle database does not support the AS keyword for table/subquery aliasing, while other databases like PostgreSQL and MySQL do. The existing BaseQuery implementation hardcoded 'as' in subquery alias generation, causing Oracle queries to fail. This change: - Replaces hardcoded 'as' with asSyntaxJoin property in BaseQuery - Oracle returns empty string for asSyntaxJoin (no AS keyword) - PostgreSQL/MySQL return 'AS' (maintains existing behavior) - Adds comprehensive Oracle query test suite validating AS syntax removal - Adds PostgreSQL regression test ensuring AS keyword is still present This fixes queries with rolling windows and multiple subqueries on Oracle, which previously generated invalid SQL like: SELECT ... FROM (...) as q_0 INNER JOIN (...) as q_1 ON ... Now Oracle correctly generates: SELECT ... FROM (...) q_0 INNER JOIN (...) q_1 ON ...
- Fix AS keyword in subquery aliases (Oracle doesn't support it) - Handle time dimensions without granularity to prevent TypeError - Implement Oracle-specific interval arithmetic using ADD_MONTHS and NUMTODSINTERVAL - Add comprehensive test suite for Oracle query generation These changes enable Oracle users to execute queries with: - Time dimension filters without granularity specification - Rolling windows and time-based calculations - Multiple subqueries with proper aliasing All changes maintain backward compatibility with other database adapters.
|
@Tankilevitch thank you for the fix! I'll review it a bit later. |
| expect(sql).toMatch(/\s+AS\s+q_0\s+/); | ||
|
|
||
| // Should NOT have pattern ) q_0 (without AS) | ||
| // This regex checks for closing paren followed by space(s), q_0, space, but NOT preceded by AS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the comment is not aligned with the regex as it doesn't include parentheses.
| */ | ||
| dimensionTimeGroupedColumn(dimension, granularity) { | ||
| // Handle case when granularity is not specified (e.g., time dimension used only for filtering) | ||
| if (!granularity) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you show the example when this function is called without granularity object?
Fix Oracle Query Generation Issues
Summary
This PR fixes three critical Oracle database compatibility issues that prevented queries with rolling windows, time dimension filters, and subqueries from executing. All fixes maintain backward compatibility with other database adapters.
Issue #1: Invalid
ASKeyword in Subquery AliasesProblem
Oracle does not support the
ASkeyword before table/subquery aliases. Queries requiring subqueries would fail with:Why It Happened
The query builder was hardcoding the
askeyword when generating subquery aliases, which works for PostgreSQL/MySQL but is invalid in Oracle SQL syntax.What We Did
Modified the query builder to use a database-specific property (
asSyntaxJoin) that each adapter can override. Oracle sets this to an empty string, while other databases use"AS".Example
PostgreSQL (unchanged):
Oracle (fixed):
Issue #2: TypeError on Time Dimensions Without Granularity
Problem
When using time dimensions for filtering only (without specifying granularity), queries would crash with:
Example Query
{ "measures": ["visitors.count"], "timeDimensions": [{ "dimension": "visitors.createdAt", "dateRange": ["2020-01-01", "2020-12-31"] // No granularity - just filtering }] }Why It Happened
The code assumed granularity was always present and tried to access properties on an undefined object. Time dimensions used only for filtering don't require granularity.
What We Did
Added a null check before accessing granularity properties. When granularity is not specified, the dimension is used as-is for filtering without grouping.
Result
Issue #3: Invalid Interval Syntax for Oracle
Problem
Queries with rolling windows would fail with:
Example Query
{ "measures": ["visitors.unboundedCount"], "timeDimensions": [{ "dimension": "visitors.createdAt", "granularity": "year", "dateRange": ["2020-01-01", "2022-12-31"] }] }(where
unboundedCounthas arollingWindowconfiguration)Why It Happened
The default date arithmetic used PostgreSQL-style interval syntax:
Oracle requires specific functions for date arithmetic instead of the
INTERVALkeyword.What We Did
Implemented Oracle-specific
addIntervalandsubtractIntervalmethods that use:ADD_MONTHS(date, n)for year/month/quarter intervalsNUMTODSINTERVAL(n, unit)for day/hour/minute/second intervalsTransformation Examples
Before (PostgreSQL syntax):
After (Oracle syntax):
Interval Conversion
ADD_MONTHS(date, ±12)ADD_MONTHS(date, ±3)ADD_MONTHS(date, ±1)date ± NUMTODSINTERVAL(n, 'DAY')date ± NUMTODSINTERVAL(n, 'HOUR')Testing
New Test Coverage
Created comprehensive test suite in
oracle-query.test.tswith 10 tests covering:ASkeyword (multiple scenarios)FETCH NEXTsyntax instead ofLIMITRegression Protection
Added test in
postgres-query.test.tsto ensure PostgreSQL continues usingASkeyword correctly.Backward Compatibility
✅ All changes are backward compatible:
ASkeywordImpact
These fixes enable Oracle users to: