hughhhh commented on code in PR #45851:
URL: https://github.com/apache/superset/pull/45851#discussion_r4458072526
##########
superset/connectors/sqla/partition_mapping.py:
##########
@@ -1851,9 +1851,38 @@ def _evaluate_with_text_forms(
# text of every correct transform too -- `CAST(:value AS DATE)`
# cannot read '2026-01-12 06:30:00' under the default
# ``NLS_DATE_FORMAT`` -- so declining here cost Oracle every time
- # range. The typed values are asked alone, a second round trip only
+ # range. The typed values are asked again, a second round trip only
# on this path, and `_declined_for_reading_by_type` checks what they
# answer against the instants they were asked about instead.
+ #
+ # In that same round trip each instant is asked once more at the top
+ # of an hour, typed and as the shorter text `_as_hour_text` writes,
+ # which a mask too short for the full text still reads. Being near
+ # the instant is not enough on its own: under an ISO date-only
+ # ``NLS_DATE_FORMAT``, ``TO_DATE(:value, 'YYYY-MM-DD HH24')`` answers
+ # midnight for 06:30, which is near it and a whole hourly key out.
+ # Those two answers differ.
+ hours = [_at_probe_hour(values[index]) for index in indexes]
+ with_hours = evaluate_transform(
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of hughhhh**
Fixed in 472a925001: you were right, and both text-form probes now skip the
refusal write, as you suggested.
- **Reproduced:** with the text refused and the typed answer cached,
`known_mirror_verdict` returned `False` after the second identical chart. That
was on the rebased branch, before the fix.
- **Fix:** the combined and hour probes pass `record_refusal=False`
([`_evaluate_with_text_forms`](https://github.com/apache/superset/blob/472a92500184d58d07bb009af429b499e6f29c58/superset/connectors/sqla/partition_mapping.py#L2151),
[`_run_probe`](https://github.com/apache/superset/blob/472a92500184d58d07bb009af429b499e6f29c58/superset/connectors/sqla/partition_mapping.py#L2373)).
A probe that answers still records as before, so the combined probe's `True`
and all-`NULL` verdicts are unchanged.
- **Broken transforms:** the typed call still records `False` when every
form is refused, because a failed probe is never cached and so always misses.
- **Tests:**
[`test_a_refused_text_form_leaves_the_verdict_to_the_typed_answer`](https://github.com/apache/superset/blob/472a92500184d58d07bb009af429b499e6f29c58/tests/unit_tests/connectors/sqla/partition_mapping_test.py#L3074)
and
[`test_a_transform_refusing_every_form_still_records_its_verdict`](https://github.com/apache/superset/blob/472a92500184d58d07bb009af429b499e6f29c58/tests/unit_tests/connectors/sqla/partition_mapping_test.py#L3104).
These use a fake engine raising ORA-01830. I didn't rerun this against a
real Oracle. Leaving this thread open for you.
##########
superset/connectors/sqla/partition_mapping.py:
##########
@@ -1851,9 +1851,38 @@ def _evaluate_with_text_forms(
# text of every correct transform too -- `CAST(:value AS DATE)`
# cannot read '2026-01-12 06:30:00' under the default
# ``NLS_DATE_FORMAT`` -- so declining here cost Oracle every time
- # range. The typed values are asked alone, a second round trip only
+ # range. The typed values are asked again, a second round trip only
# on this path, and `_declined_for_reading_by_type` checks what they
# answer against the instants they were asked about instead.
+ #
+ # In that same round trip each instant is asked once more at the top
+ # of an hour, typed and as the shorter text `_as_hour_text` writes,
+ # which a mask too short for the full text still reads. Being near
+ # the instant is not enough on its own: under an ISO date-only
+ # ``NLS_DATE_FORMAT``, ``TO_DATE(:value, 'YYYY-MM-DD HH24')`` answers
+ # midnight for 06:30, which is near it and a whole hourly key out.
+ # Those two answers differ.
+ hours = [_at_probe_hour(values[index]) for index in indexes]
+ with_hours = evaluate_transform(
+ database,
+ catalog,
+ schema,
+ transform,
+ values + hours + [_as_hour_text(hour) for hour in hours],
+ )
+ if with_hours is not None:
+ typed_hours = with_hours[len(values) : len(values) + len(hours)]
+ text_hours = with_hours[len(values) + len(hours) :]
+ text_answers.extend(
+ (index, TextRefused(hour, typed_hour, text_hour))
+ for index, hour, typed_hour, text_hour in zip(
+ indexes, hours, typed_hours, text_hours, strict=False
+ )
+ )
+ return True, with_hours[: len(values)]
+ # The shorter text is refused too -- every ISO spelling is, under
+ # Oracle's default ``NLS_DATE_FORMAT`` -- so the typed values are
+ # asked alone, and only the distance from the instant is checked.
typed = evaluate_transform(database, catalog, schema, transform,
values)
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of hughhhh**
Fixed in 472a925001: the column-limit path was real, so the hour check no
longer shares a statement with the filter values.
- **Reproduced:** with a fake engine that raises ORA-01792 past 1,000
columns, 334 days made 1,002 projections. Midnight answers then passed the
distance check and the mirror was emitted.
- **Fix:** [one instant is now asked on its own two-column
query](https://github.com/apache/superset/blob/472a92500184d58d07bb009af429b499e6f29c58/superset/connectors/sqla/partition_mapping.py#L2183),
after the typed values. Losing the time of day doesn't depend on the day, so
one sample is enough, and the check no longer depends on list size. A list of
any length now declines in this case.
- **Test:**
[`test_a_long_list_still_has_its_refused_text_form_checked_at_the_hour`](https://github.com/apache/superset/blob/472a92500184d58d07bb009af429b499e6f29c58/tests/unit_tests/connectors/sqla/partition_mapping_test.py#L3022).
**Not changed:** if the small hour query itself fails, that still counts as
the text being refused and only the distance check applies. Every ISO spelling
is refused under Oracle's default `NLS_DATE_FORMAT`, so declining there would
cost correct transforms their pruning. That means a transient failure of that
query can still skip the hour check for that chart.
The 1,000-column limit is simulated here, not run against a real Oracle.
--
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]
