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
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 👍 / 👎.
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 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c19112bc66
ℹ️ 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".
| # re-import -- is preserved. Valid + lossless. | ||
| if used_names is not None and name in used_names: | ||
| i = 2 | ||
| while f"{stem}_{i}_{tf_suffix}" in used_names: |
There was a problem hiding this comment.
When a collision is disambiguated before a later time-group base is processed, used_names does not yet include fields that that later dimension_group will generate. For example, started and started_hour on different SQL sources plus an existing started_2_hour time dimension makes this loop choose started_2_hour for the standalone collision, and the later dimension_group: started_2 { timeframes: [hour] } also generates started_2_hour, leaving duplicate LookML fields and causing one source to be ambiguous or overwritten on import. Seed used_names with all pending generated time field names, or check future group outputs before selecting the _2 suffix.
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.
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.