Conversation
- Convert edr_dateadd to use adapter.dispatch for adapter-specific overrides - Add dremio__edr_dateadd that fixes dbt-dremio's dateadd bugs: 1. Casts interval to string before calling .replace() (fixes integer interval) 2. Outputs TIMESTAMPADD expression directly (no scalar subquery wrapper) - Update dremio__edr_cast_as_timestamp to normalize ISO 8601 timestamps: 1. Remove 'T' date-time separator (Gandiva can't parse it) 2. Remove 'Z' UTC suffix (Gandiva rejects unknown zone) 3. Truncate sub-millisecond precision Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
👋 @devin-ai-integration[bot] |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an adapter-dispatched Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…, handle uppercase ORDER BY 1 Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
| {% macro dremio__edr_dateadd(datepart, interval, from_date_or_timestamp) %} | ||
| {% set datepart = datepart | lower %} | ||
| {% set interval = interval | string %} | ||
| {% set interval = interval.replace('order by 1', '').replace('ORDER BY 1', '') %} |
There was a problem hiding this comment.
Not sure I understand this. Why would the interval contain order by 1?
This feels like a hack
There was a problem hiding this comment.
You're right, this is a hack from dbt-dremio's original dremio__dateadd macro (line 32 of dbt/include/dremio/macros/utils/date_spine.sql). Since our edr_dateadd uses adapter.dispatch and bypasses dbt-dremio's macro entirely, the interval will never contain order by 1 here — it was only relevant in dbt-dremio's own recursive calls. Removing it.
| {%- macro dremio__edr_cast_as_timestamp(timestamp_field) -%} | ||
| cast(REGEXP_REPLACE({{ timestamp_field }}, '(\.\d{3})\d+', '$1') as {{ elementary.edr_type_timestamp() }}) | ||
| cast( | ||
| REGEXP_REPLACE( |
There was a problem hiding this comment.
Do we really need three regexp_replace?
Can't we replace with one?
There was a problem hiding this comment.
Good point — consolidating into a single REGEXP_REPLACE with alternation pattern.
There was a problem hiding this comment.
I tried consolidating into one REGEXP_REPLACE with alternation ((\d)T(\d)|(\.\d{3})\d+|Z$), but it doesn't work because each branch needs a different replacement string (space for T, truncation for sub-ms, empty for Z). Dremio's REGEXP_REPLACE only takes a single replacement string, and unmatched group backreferences produce empty strings — so '$1$3 $2' adds spurious spaces when only the sub-ms or Z branch matches.
Updated the comment to explain this and added a concrete example: '2024-01-15T12:30:00.123456Z' → '2024-01-15 12:30:00.123'.
…mprove comments - Remove unnecessary 'order by 1' stripping from dremio__edr_dateadd (not needed since adapter.dispatch bypasses dbt-dremio's macro entirely) - Keep three REGEXP_REPLACE calls since each has a different replacement pattern (space, truncation, removal) that can't be combined into a single call - Add concrete example in comment showing the transformation Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
…tamp Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
Summary
Moves Dremio-specific macro overrides from the elementary CLI project into dbt-data-reliability where they belong.
edr_dateadd: Refactored to useadapter.dispatch(matching the pattern used by otheredr_*macros likeedr_cast_as_timestamp). The existing logic moves todefault__edr_dateadd. A newdremio__edr_dateaddis added to work around two bugs in dbt-dremio's nativedateadd:.replace()on the interval parameter which fails when interval is an integerselect TIMESTAMPADD(...)creating$SCALAR_QUERYerrors when embedded in larger expressionsdremio__edr_cast_as_timestamp: Enhanced with two additionalREGEXP_REPLACEcalls to normalize ISO 8601 timestamps that Dremio's Gandiva engine cannot parse — specifically theTdate-time separator andZUTC suffix.Review & Testing Checklist for Human
adapter.dispatchrefactor doesn't regress non-Dremio adapters. The structural change from directdbt.dateaddcall toadapter.dispatch('edr_dateadd', 'elementary')affects all adapters. Confirm thatdefault__edr_dateaddis correctly resolved for Postgres, Snowflake, BigQuery, etc.dremio__edr_dateaddcovers all dateparts used in the codebase. Currently supports: year, quarter, month, week, day, hour, minute, second. Any other datepart (e.g.millisecond) will raise a compiler error.dremio__edr_cast_as_timestamp— the regex chain'(\d)T(\d)' → '$1 $2','(\.\d{3})\d+' → '$1','Z$' → ''should correctly normalize ISO 8601 toYYYY-MM-DD HH:MM:SS.sss.Notes
interval.replace('order by 1', '')indremio__edr_dateaddis a workaround for dbt-dremio appendingorder by 1to its scalar subquery output — this is fragile but necessary until dbt-dremio fixes itsdateaddmacro upstream.Summary by CodeRabbit
New Features
Improvements