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
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 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
5af0072 to
50ef09e
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
9d82713 to
a59ef39
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
50ef09e to
1dfe950
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
a59ef39 to
ee71964
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee719641aa
ℹ️ 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".
| if timeframe not in seen_timeframes: | ||
| seen_timeframes.add(timeframe) | ||
| timeframes.append(timeframe) |
There was a problem hiding this comment.
When an imported LookML group contains both time and second, both dimensions now have granularity == "second" and map to the same exported timeframe. Fresh evidence in the current revision is this new de-duplication branch: after created_time adds time, created_second is skipped, so exporting timeframes: [time, second, hour] writes only [time, hour] and re-import loses created_second entirely. Preserve the original LookML timeframe (for example in metadata) or emit second for the *_second member instead of de-duping it away.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
1dfe950 to
0a99120
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
ee71964 to
df776a1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df776a1ceb
ℹ️ 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.
_second dimensions on export
When exporting a Sidemantic model that was not just parsed from LookML, second-grain dimensions have no meta['lookml_timeframe'], so this fallback maps them to LookML time. Because the new suffix stripping groups created_second under base created, export writes timeframes: [time], and a re-import exposes created_time instead of created_second; native second-grain fields therefore still do not round-trip despite the new _second handling.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
0a99120 to
276e8e2
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
df776a1 to
b3fd7f9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3fd7f9d39
ℹ️ 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".
| timeframe = (dim.meta or {}).get("lookml_timeframe") or granularity_mapping.get( | ||
| dim.granularity, "date" | ||
| ) |
There was a problem hiding this comment.
_time second-grain exports
When exporting a native or edited Sidemantic model with a second-grain dimension named like created_time and no meta['lookml_timeframe'], this fallback always emits the second timeframe after the base-name stripping above removes _time. The exported LookML becomes dimension_group: created { timeframes: [second] }, so re-import exposes created_second and drops/renames the original created_time field; use the _time suffix as the fallback timeframe for second-grain fields when no original LookML metadata is present.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
276e8e2 to
0966fe7
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
b3fd7f9 to
958456a
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
0966fe7 to
ec74f18
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
958456a to
ce7a884
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
ec74f18 to
e72acbd
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
ce7a884 to
c8f1445
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8f144537f
ℹ️ 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".
| for suffix in ["_date", "_week", "_month", "_quarter", "_year", "_time", "_hour", "_minute", "_second"]: | ||
| if dim.name.endswith(suffix): | ||
| base_name = dim.name[: -len(suffix)] |
There was a problem hiding this comment.
When a native model has same-prefix time dimensions backed by different columns, e.g. started_date on {model}.started_at and started_second on {model}.other_at, stripping _second groups both under one dimension_group. The export then writes only the first group's SQL, so a round-trip rewires started_second to the date dimension's source column. Keep groups split by source SQL, or only strip the new suffixes when the candidate dimensions share the same SQL.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
c8f1445 to
32608f9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8fb8fbffa
ℹ️ 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".
| for tf in sorted(self._TIME_GRANULARITY_TIMEFRAMES, key=len, reverse=True): | ||
| if dim.name.endswith("_" + tf): | ||
| timeframe = tf |
There was a problem hiding this comment.
When a native Sidemantic time dimension has no meta['lookml_timeframe'], this branch trusts any LookML-looking suffix even if it contradicts dim.granularity. For example, Dimension(name='created_time', granularity='hour') now exports as timeframes: [time]; since this commit imports time as second, a LookML round-trip changes the field to second-grain and queries group by seconds instead of hours. Only derive the timeframe from the suffix when it maps to the stored granularity, otherwise use the granularity mapping or an explicit standalone form.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
417f230 to
f051bbf
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
a8fb8fb to
7291ef9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7291ef9166
ℹ️ 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".
| m = cls._DATE_TRUNC_RE.match(src) | ||
| if m and m.group(1).lower() == grain: | ||
| trunc_sql = src | ||
| else: | ||
| trunc_sql = f"DATE_TRUNC('{grain}', {src})" |
There was a problem hiding this comment.
When same-prefix time dimensions use different source SQLs and the colliding field is a preserved LookML millisecond or microsecond timeframe, this fallback truncates with the stored Sidemantic grain (second). For example, started_date on one column plus started_millisecond with meta['lookml_timeframe'] == 'millisecond' exports started_millisecond as DATE_TRUNC('second', ...), so the generated LookML loses the sub-second precision even though the field name/metadata still says millisecond. The minute15/30 special case above handles one class where the LookML timeframe is more specific than dim.granularity; the same preservation is needed for sub-second timeframes.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
7291ef9 to
0201ffe
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0201ffe114
ℹ️ 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".
| if m and m.group(1).lower() == grain: | ||
| trunc_sql = src | ||
| else: | ||
| trunc_sql = f"DATE_TRUNC('{grain}', {src})" |
There was a problem hiding this comment.
When same-prefix time dimensions use different SQL and the colliding field came from LookML time_of_day, this fallback uses the stored Sidemantic grain (hour) and exports DATE_TRUNC('hour', ...). Looker documents time_of_day as a time-of-day value such as 17:15, distinct from the hour bucket, so exporting started_date on one column plus started_time_of_day on another rewrites the latter to hourly buckets and Looker queries lose the minute/second component; use the stored timeframe to emit an equivalent time-of-day expression or another form that preserves that timeframe. See https://docs.cloud.google.com/looker/docs/reference/param-field-dimension-group#timeframe_options.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
f051bbf to
d1a7eb4
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
0201ffe to
b722ca9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b722ca9669
ℹ️ 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".
| for tf in sorted(self._TIME_GRANULARITY_TIMEFRAMES, key=len, reverse=True): | ||
| if dim.name.endswith("_" + tf) and self._TIME_GRANULARITY_TIMEFRAMES[tf] == dim.granularity: |
There was a problem hiding this comment.
When a native Sidemantic time dimension has no meta['lookml_timeframe'], this suffix check treats any suffix whose coarse mapping equals dim.granularity as safe to export. For example, Dimension(name='created_minute15', granularity='minute', sql='ts') now exports timeframes: [minute15], so the generated LookML buckets into 15-minute intervals even though the source model's grain is one minute; the same semantic widening/narrowing applies to minute30, millisecond/microsecond, and time_of_day, which all collapse to coarser Sidemantic grains in the mapping. Only infer exact one-to-one suffixes for native dims, or require preserved LookML metadata for these special timeframes.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
d1a7eb4 to
e093d18
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
b722ca9 to
9ae20b9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ae20b9334
ℹ️ 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".
| name.endswith("_" + tf) | ||
| and g == grain | ||
| and (not recovered_timeframe or len(tf) > len(recovered_timeframe)) |
There was a problem hiding this comment.
When a native/edited model has no meta['lookml_timeframe'] and same-prefix time dimensions use different SQL sources, _export_collision_time_dim writes a plain DATE_TRUNC('minute', ...) for a field like started_minute15. This recovery then sees the _minute15 suffix and records lookml_timeframe='minute15', so the next export turns the original one-minute dimension into a 15-minute bucket; the same false metadata inference applies to other inexact suffixes such as minute30/subsecond/time_of_day. Only recover those special timeframes from the explicit bucket/subsecond forms that actually preserve their semantics, not from a plain DATE_TRUNC whose grain merely matches the coarse mapping.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
e093d18 to
8110430
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
9ae20b9 to
9d325f5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d325f523b
ℹ️ 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".
| trunc_sql = src | ||
| else: | ||
| trunc_sql = f"DATE_TRUNC('{grain}', {src})" | ||
| return {"name": dim.name, "type": "date_time", "sql": trunc_sql} |
There was a problem hiding this comment.
When a native colliding time dimension does not end in a LookML timeframe suffix, this standalone export is not recoverable by the importer. For example, started at hour grain alongside started_date on a different SQL source can export as dimension: started { type: date_time sql: DATE_TRUNC('hour', ...) }, but _parse_dimension only restores time granularity when the name ends with _<timeframe>, so a round-trip turns started into a categorical dimension with no granularity and breaks time queries/filters on that field.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
8110430 to
9202c25
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
9d325f5 to
f332ebb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f332ebb498
ℹ️ 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".
| if not recoverable: | ||
| suffix = grain if grain in cls._SUBSECOND_TF else cls._GRAIN_TO_TIMEFRAME.get(grain, "date") | ||
| name = f"{name}_{suffix}" |
There was a problem hiding this comment.
When a suffixless colliding time dimension is exported and another field already has the canonical suffixed name, this blind append emits duplicate LookML field names. For example, a model with started at hour grain plus an existing started_hour dimension and a separate started_date source exports two dimension: started_hour blocks; on import the lookup for duplicate names keeps the last SQL, so one field/source is lost or the LookML is ambiguous. Check existing emitted names before synthesizing the suffix or use a form that preserves uniqueness.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
9202c25 to
33ee7f7
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
f332ebb to
b1584ca
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1584ca43a
ℹ️ 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".
| recoverable = (grain in cls._SUBSECOND_TF and name.endswith("_" + grain)) or any( | ||
| name.endswith("_" + t) and g == grain for t, g in cls._TIME_GRANULARITY_TIMEFRAMES.items() | ||
| ) |
There was a problem hiding this comment.
When a suffixless time dim wins the dimension_group slot and another colliding dim already has the generated suffix, e.g. started at hour grain plus started_hour on a different SQL source, this recoverable path returns started_hour unchanged and bypasses any used_names collision handling. Fresh evidence in this revision is that the duplicate now comes from dimension_group: started generating started_hour plus a standalone dimension: started_hour, so LookML import/querying can overwrite or ambiguously resolve one of the sources.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
33ee7f7 to
64fefb4
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
b1584ca to
6921c87
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6921c87fee
ℹ️ 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".
| if used_names is not None and name in used_names: | ||
| i = 2 | ||
| while f"{name}_{i}" in used_names: | ||
| i += 1 | ||
| name = f"{name}_{i}" |
There was a problem hiding this comment.
Fresh evidence in this revision is the _2 disambiguation here: when a collision is avoided by renaming a recoverable field such as started_hour to started_hour_2, _parse_dimension no longer sees a trailing LookML timeframe suffix and imports the DATE_TRUNC('hour', ...) standalone as a categorical dimension. For example, a native model with started (hour), started_hour (hour on a different SQL), and started_date exports a dimension: started_hour_2, so the original started_hour source loses its time granularity after one round-trip; use a recoverable disambiguated shape such as <base>_2_<timeframe> or carry explicit recovery metadata.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
64fefb4 to
c57bdda
Compare
nicosuave
force-pushed
the
fix/lookml-time-timeframe-precision
branch
from
6921c87 to
ef8e585
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef8e585264
ℹ️ 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".
| f"DATE_TRUNC('hour', {src}) + INTERVAL '1 minute' * " | ||
| f"CAST(FLOOR(EXTRACT(MINUTE FROM {src}) / {n}) * {n} AS INTEGER)" |
There was a problem hiding this comment.
Fresh evidence after the prior // fix is that _minute_bucket_sql still hard-codes PostgreSQL-style DATE_TRUNC('hour', ...) + INTERVAL '1 minute' * .... When exporting a LookML project backed by BigQuery and a minute15/minute30 field takes the collision path, Looker sends this SQL directly to BigQuery; BigQuery's timestamp truncation syntax is TIMESTAMP_TRUNC(timestamp_value, timestamp_granularity) and interval arithmetic uses INTERVAL n MINUTE, so the exported field fails at query time. Build this expression through the dialect-aware generator or avoid raw warehouse-specific SQL for these standalone dimensions. See https://cloud.google.com/bigquery/docs/reference/standard-sql/timestamp_functions#timestamp_trunc.
Useful? React with 👍 / 👎.
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.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
c57bdda to
589cca7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee425a6e43
ℹ️ 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".
| if n is not None: | ||
| bm = cls._MINUTE_BUCKET_RE.match(src) | ||
| bucket_sql = src if (bm and int(bm.group(2)) == n) else cls._minute_bucket_sql(src, n) | ||
| return {"name": dim.name, "type": "date_time", "sql": bucket_sql} |
There was a problem hiding this comment.
When a native or edited model has a colliding minute15/minute30 time dimension whose name is already generated by the winning dimension_group, this early return bypasses the used_names collision handling below. For example, a suffixless started dimension with meta['lookml_timeframe'] == 'minute15' can export dimension_group: started { timeframes: [minute15] } while a sibling started_minute15 on another SQL source also exports as dimension: started_minute15, leaving duplicate LookML field names and causing one source to be overwritten/dropped on re-import; run this bucket path through the same suffix synthesis and uniqueness logic before returning.
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.