Preserve sub-hour precision for LookML 'time' timeframe#245
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 360068606c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "second": "time", | ||
| "minute": "minute", | ||
| "hour": "time", |
There was a problem hiding this comment.
When this mapping is used for an hour-grain dimension, it still emits the LookML time timeframe even though this commit now imports time as second. Any exported model containing an hour-granularity dimension, including one originally parsed from a LookML hour timeframe, will re-import as a second-grain *_time dimension instead of preserving hourly buckets; export hour as the LookML hour timeframe to avoid silently increasing query granularity.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
84410ae to
92c08af
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
3600686 to
2e5a5e9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e5a5e9068
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "second": "time", | ||
| "minute": "minute", |
There was a problem hiding this comment.
When these new mappings are used for LookML-generated *_minute or *_second dimensions, they still do not round-trip because the base-name extraction above does not strip those suffixes, and second is always emitted as the time timeframe. For an imported dimension_group: start { timeframes: [minute, second] }, export groups the fields under start_minute/start_second, so re-import exposes start_minute_minute and start_second_time instead of the original fields; add suffix-aware handling or preserve the original timeframe before treating these grains as round-trippable.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
92c08af to
d2cb276
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
2e5a5e9 to
3160319
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
d2cb276 to
5efe508
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
3160319 to
5efe508
Compare
Looker's 'time' timeframe keeps full timestamp precision (to the second), but the adapter mapped it to hour granularity, silently collapsing all sub-hour rows. Map 'time' to second granularity, and add second/minute to the export granularity->timeframe map so they round-trip instead of degrading to 'date'. Update the four tests that asserted the old hour grain.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d82713acb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Map granularity to timeframe | ||
| granularity_mapping = { | ||
| "hour": "time", | ||
| "second": "time", |
There was a problem hiding this comment.
When a LookML dimension_group contains a second timeframe, it imports as a *_second dimension with granularity == "second", but this export mapping writes every second-grain dimension back as the LookML time timeframe. Fresh evidence in this revision is that *_second is now grouped with the same base name as *_time, so a group with both time and second exports duplicate timeframes: [time, time]; re-importing drops created_second and creates duplicate created_time fields, corrupting round-trip models that use Looker's second timeframe.
Useful? React with 👍 / 👎.
1 participant
Summary
Part of the LookML adapter correctness series. Stacked on #244 (base =
fix/lookml-default-view-table).Looker's
timetimeframe returns the timestamp to the second, but the adapter mapped it to hour granularity (DATE_TRUNC('hour', ...)), silently collapsing every sub-hour row into one.Changes
timetimeframe tosecondgranularity (sidemantic supports it).second/minuteto the export granularity->timeframe map so they round-trip instead of degrading todate.hourgrain, and add a dedicated test.Not included (deferred)
minute15/minute30(no native 15/30-min granularity in sidemantic) andtime_of_day(clock-time extraction) remain approximations — documented in the audit.