geido commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4361509756
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -1365,19 +1695,76 @@ function DatasourceEditor({
try {
const newCols = await fetchSyncedColumns(datasource, signal);
+ // Everything past the `await` reads the refs rather than the closure;
+ // see their declaration.
+ const liveColumns = currentColumnsRef.current;
+ const liveDatasource = datasourceRef.current;
+
const columnChanges = updateColumns(
- datasource.columns,
+ liveColumns,
Review Comment:
I think merging against `liveColumns` has a side effect with the flag off.
If I delete a column row that still exists in the source and then sync, it
comes back with a `nanoid()` id, so the save fails with "One or more columns
already exist" (master reused the old row and saved fine). Could the merge fall
back to `datasource.columns` for names missing from `liveColumns`?
[mp4](https://gist.githubusercontent.com/geido/2e9a0de274464f8bac4b8ac43e8888e9/raw/bug2-sync-after-delete.mp4)
##########
superset/db_engine_specs/base.py:
##########
@@ -580,6 +580,47 @@ def normalize_custom_sql_metric(cls, expression: str) ->
str:
allows_joins = True
allows_subqueries = True
allows_alias_in_select = True
+ # Appended to a ``SELECT`` that has no ``FROM`` -- evaluating expressions
+ # and nothing else. Most engines accept a bare ``SELECT 1``; Oracle, DB2
+ # and Db2 for i require a one-row table to select it from.
+ select_without_from_suffix: str = ""
+ # Whether ``=`` on a text column compares byte-exactly by default: no case
+ # folding, no trailing-space padding, no accent insensitivity.
+ #
+ # Partition filter mapping needs this. It mirrors ``col = v`` onto
+ # ``partition_col = T(v)`` on the strength of ``col = v`` implying
+ # ``T(col) = T(v)``, which holds for value equality and not for SQL
+ # equality: under a case-insensitive collation a stored ``'us'`` satisfies
+ # a filter for ``'US'``, while the mirror ``hex('US')`` excludes the row,
+ # and the chart silently loses it. False by default, so a spec that has
+ # not said so does not mirror string equality.
+ #
+ # This speaks for the engine's *default* comparison only. A column
+ # declaring its own non-binary collation (``country COLLATE NOCASE``) is
+ # invisible to Superset -- nothing in SQLAlchemy's reflection or the engine
+ # specs exposes it -- so on such a column the assumption remains the
+ # owner's, like ``p = T(mapped_col)`` itself.
+ binary_string_comparison: bool = False
+
+ # Every text form this engine reads as a date or timestamp when one is
+ # compared against a temporal column. `strftime`/`strptime` patterns.
+ #
+ # Partition filter mapping needs this. A bucketing key such as
+ # ``to_char(:value, 'YYYYMMDD')`` is legitimately text, so the mirror
+ # ``part_date = '20260115'`` has to be allowed against a ``DATE`` partition
+ # column -- but only where the engine reads it, because comparing a
+ # temporal column against text is an implicit coercion an engine need not
+ # have. Trino has no VARCHAR-to-DATE comparison and BigQuery coerces only a
+ # canonical ``YYYY-MM-DD`` *literal*, so on those even plain ISO text fails
+ # the whole chart rather than merely losing the pruning -- which is why the
+ # ISO forms are declared here too rather than assumed universal.
+ #
+ # Empty by default, so an engine Superset has not confirmed declines the
+ # mirror and keeps the query working. Declared on concrete specs, not on a
+ # shared base: `PostgresBaseEngineSpec` is inherited by Snowflake, whose
+ # AUTO date input reads an all-digit string as epoch seconds instead.
+ temporal_literal_formats: tuple[str, ...] = ()
Review Comment:
IIUC this guard only covers `str` results, while a `date`/`datetime` result
is written by the Trino dialect as the same bare string. On Trino,
`date(CAST(:value AS TIMESTAMP))` onto a DATE `dt` previews as valid, then the
chart fails with "Cannot apply operator: date <= varchar(10)" (`=`/`IN` too).
Could these get the `_reads_as_temporal` check, or a typed literal, so only
pruning is lost?
[mp4](https://gist.githubusercontent.com/geido/2e9a0de274464f8bac4b8ac43e8888e9/raw/bug1-trino-date-partition.mp4)
##########
superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx:
##########
@@ -420,7 +422,12 @@ export default function DateFilterLabel(props:
DateFilterControlProps) {
{...props}
onDescriptionHoverChange={setIsDescriptionHovered}
/>
- {popoverContent}
+ {/* The glyph sits outside the popover trigger so clicking it opens the
+ tooltip rather than the time-range editor. */}
+ <Flex align="center" gap={theme.sizeUnit}>
Review Comment:
I think this `Flex` changes the layout with the flag off too. The trigger
span becomes a flex item, so the Time Range box in the ad-hoc filter popover
shrinks to its label instead of filling the popover like on master (250px vs
320px here). Would rendering the `Flex` only when `partitionMapping` is set
keep the old markup?
[gif](https://gist.githubusercontent.com/geido/2e9a0de274464f8bac4b8ac43e8888e9/raw/bug3-time-range-width.gif)
ยท
[mp4](https://gist.githubusercontent.com/geido/2e9a0de274464f8bac4b8ac43e8888e9/raw/bug3-time-range-width.mp4)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
