You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "malon64 (via GitHub)" <gi...@apache.org> on 2026/06/15 15:03:35 UTC
[PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
malon64 opened a new pull request, #41055:
URL: https://github.com/apache/superset/pull/41055
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
For Apache Iceberg tables queried through Trino, `database.get_indexes(table)` returns the columns of the Trino `"<table>$partitions"` system table — which for Iceberg are **metadata fields** (`record_count`, `file_count`, `total_size`, `data`, and a nested `partition` ROW), *not* user partition keys. Superset treated them as partition columns and fed them into `latest_partition()` / `_partition_query()`, generating invalid SQL such as:
```sql
SELECT * FROM gold."mart_customer_health_score$partitions"
ORDER BY data DESC, file_count DESC, record_count DESC, total_size DESC
LIMIT 1
```
which breaks table preview and latest-partition filtering. Reproducible through `apache/superset:6.1.0`. Fixes #25307 (and dup #26449).
**Fix.** `TrinoEngineSpec` now filters these out before building partition metadata. A partition index is treated as Iceberg metadata — and dropped — only when **both**:
1. it contains the signature columns `{record_count, file_count, total_size}` (always present on Iceberg `$partitions`), **and**
2. *every* one of its columns is a known Iceberg metadata field.
Condition 2 is what makes it safe: an index with any real partition key (e.g. a Hive table partitioned on `ds` that also happens to have a `record_count` column) fails the check and is passed through completely untouched. The filter is applied in `get_extra_table_metadata()` and in an override of `latest_partition()`, so the table-preview path, the `{{ latest_partition() }}` macro, and automatic `where_latest_partition()` filtering are all covered.
**Why filter the fields instead of asking Trino for the table type?** We deliberately chose not to detect the Iceberg connector via `system.metadata.catalogs`:
- **Durability** — `connector_name` only exists on recent Trino versions (older releases expose `connector_id`, which is just the catalog name and doesn't identify the connector), so a type lookup would be version-fragile and silently wrong on older clusters.
- **No extra round-trip / permissions** — the heuristic is purely local to the reflection data Superset already has. A type query adds a round-trip on a UI metadata path and assumes the principal can read the `system` catalog, which some locked-down deployments restrict.
- **Same outcome** — even after confirming a table is Iceberg, the correct action is identical to what we do here: skip `$partitions`-based latest-partition logic. So a connector lookup would add cost and fragility without changing behavior.
- **Connector-agnostic & non-invasive** — the change only ever *removes* an all-metadata index; it cannot affect Hive/Presto or any non-Iceberg table, and it stays consistent with the engine spec's existing reflection-based design.
Trade-off: a genuinely partitioned Iceberg table now shows *no* latest-partition metadata rather than wrong metadata (matching the issue's suggested direction). Reading partition values out of the nested `partition` ROW is a larger, separate enhancement.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend behavior; no UI change).
### TESTING INSTRUCTIONS
Unit tests added in `tests/unit_tests/db_engine_specs/test_trino.py` cover: Iceberg unpartitioned, Iceberg partitioned, a real key alongside coincidental signature-named columns (preserved), and Hive (unchanged).
```bash
pytest tests/unit_tests/db_engine_specs/test_trino.py
```
Manual: connect a Trino catalog backed by the Iceberg connector, open an Iceberg table in SQL Lab / dataset preview, and confirm it loads without an `ORDER BY data DESC, file_count DESC, …` error. Confirm a Hive partitioned table still shows its latest partition.
### ADDITIONAL INFORMATION
- [x] Has associated issue: #25307
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #41055:
URL: https://github.com/apache/superset/pull/41055#issuecomment-4709476568
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 0b4e5df7e8ca65a2bd634463b5449635af76c6f2 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a30182588b9a1000897a9c4 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-41055--superset-docs-preview.netlify.app](https://deploy-preview-41055--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpLCoAgEADQu8w6yf7aDTrGqGMJlSITFNHdc_cW74WU45EYZoAKTMbTbsU-3LUPO1MW8WIRLBnKqziI0SFjqY7SHp_FlTxiJxvVDkoZjY2UUukJte3h-wE) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-41055--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "malon64 (via GitHub)" <gi...@apache.org>.
malon64 commented on code in PR #41055:
URL: https://github.com/apache/superset/pull/41055#discussion_r3414643668
##########
superset/db_engine_specs/trino.py:
##########
@@ -631,3 +680,34 @@ def get_indexes(
return super().get_indexes(database, inspector, table)
except NoSuchTableError:
return []
+
+ @classmethod
+ def latest_partition(
+ cls,
+ database: Database,
+ table: Table,
+ show_first: bool = False,
+ indexes: list[dict[str, Any]] | None = None,
+ ) -> tuple[list[str], list[str] | None]:
+ """
+ Return the latest partition for a table.
+
+ Iceberg "$partitions" metadata fields are filtered out first, so we
+ never build a latest-partition query against them.
+
+ :param database: the database the query will be run against
+ :param table: the table instance
+ :param show_first: return the value for the first partitioning key when
+ there are several
+ :param indexes: the indexes associated with the table
+ :returns: the column names and the latest partition values
+ """
+ if indexes is None:
+ indexes = database.get_indexes(table)
+
+ return super().latest_partition(
+ database,
+ table,
+ show_first=show_first,
+ indexes=cls._filter_iceberg_partition_indexes(indexes),
+ )
Review Comment:
Fixed by memoizing the override with the same timeout as the base method, so
the index lookup and filtering only run on a cache miss — restoring the
original caching cadence.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41055:
URL: https://github.com/apache/superset/pull/41055#issuecomment-4709306691
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41055:
URL: https://github.com/apache/superset/pull/41055#discussion_r3414536056
##########
superset/db_engine_specs/trino.py:
##########
@@ -631,3 +680,34 @@ def get_indexes(
return super().get_indexes(database, inspector, table)
except NoSuchTableError:
return []
+
+ @classmethod
+ def latest_partition(
+ cls,
+ database: Database,
+ table: Table,
+ show_first: bool = False,
+ indexes: list[dict[str, Any]] | None = None,
+ ) -> tuple[list[str], list[str] | None]:
+ """
+ Return the latest partition for a table.
+
+ Iceberg "$partitions" metadata fields are filtered out first, so we
+ never build a latest-partition query against them.
+
+ :param database: the database the query will be run against
+ :param table: the table instance
+ :param show_first: return the value for the first partitioning key when
+ there are several
+ :param indexes: the indexes associated with the table
+ :returns: the column names and the latest partition values
+ """
+ if indexes is None:
+ indexes = database.get_indexes(table)
+
+ return super().latest_partition(
+ database,
+ table,
+ show_first=show_first,
+ indexes=cls._filter_iceberg_partition_indexes(indexes),
+ )
Review Comment:
**Suggestion:** This override performs `database.get_indexes(table)` before calling the memoized parent implementation, so callers that omit `indexes` (for example Jinja `latest_partition()`/`where_latest_partition()` paths) will hit metadata introspection on every call even when the partition result is cached. That regresses the cache behavior and adds unnecessary round-trips. Keep the override memoized (or otherwise move index lookup/filtering into the cached path) so repeated calls do not re-fetch indexes each time. [cache]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Trino table metadata API always re-runs index introspection.
- ⚠️ Jinja `latest_partition` calls on Trino lose index caching.
- ⚠️ Extra Trino metadata queries can slow dashboards using partitions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a Trino-backed dataset's metadata/preview in the UI, which uses
`database.select_star` with `latest_partition=True` as implemented in
`superset/databases/utils.py:100-27` (the `"selectStar"` field calls
`database.select_star(..., latest_partition=True)` for the inspected table).
2. That call is routed to `BaseEngineSpec.select_star` in
`superset/db_engine_specs/base.py:51-102`, which for `latest_partition=True` invokes
`cls.where_latest_partition(database, table, qry, columns=cols)`, and for Trino databases
`cls` is `TrinoEngineSpec`.
3. `PrestoBaseEngineSpec.where_latest_partition` at
`superset/db_engine_specs/presto.py:13-48` calls `cls.latest_partition(database, table,
show_first=True)`; for Trino this resolves to `TrinoEngineSpec.latest_partition` defined
at `superset/db_engine_specs/trino.py:685-713`, which first executes `if indexes is None:
indexes = database.get_indexes(table)` (line 705-706).
4. `TrinoEngineSpec.latest_partition` then delegates to the memoized parent
`PrestoBaseEngineSpec.latest_partition` at `superset/db_engine_specs/presto.py:57-78`, but
because `database.get_indexes(table)` is executed in the subclass *before* the memoized
call, every UI refresh or repeated Jinja macro call (see `superset/jinja_context.py:37-43`
using `latest_partition`) on the same Trino table re-triggers expensive
`database.get_indexes(table)` introspection even when the parent `latest_partition` result
is served from cache, regressing the original caching behavior where index lookup occurred
inside the memoized function body.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=dd4e6541cf184a708d25315aeb4d2d6c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=dd4e6541cf184a708d25315aeb4d2d6c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/trino.py
**Line:** 705:713
**Comment:**
*Cache: This override performs `database.get_indexes(table)` before calling the memoized parent implementation, so callers that omit `indexes` (for example Jinja `latest_partition()`/`where_latest_partition()` paths) will hit metadata introspection on every call even when the partition result is cached. That regresses the cache behavior and adds unnecessary round-trips. Keep the override memoized (or otherwise move index lookup/filtering into the cached path) so repeated calls do not re-fetch indexes each time.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41055&comment_hash=142368071144f1646cbb73b51532219a8d06e282d09638097f12830fa268b5ca&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41055&comment_hash=142368071144f1646cbb73b51532219a8d06e282d09638097f12830fa268b5ca&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41055:
URL: https://github.com/apache/superset/pull/41055#discussion_r3454152497
##########
superset/db_engine_specs/trino.py:
##########
@@ -631,3 +680,66 @@ def get_indexes(
return super().get_indexes(database, inspector, table)
except NoSuchTableError:
return []
+
+ @classmethod
+ @cache_manager.data_cache.memoize(timeout=60)
+ def latest_partition(
+ cls,
+ database: Database,
+ table: Table,
+ show_first: bool = False,
+ indexes: list[dict[str, Any]] | None = None,
Review Comment:
This one's a false alarm, I think... `latest_partition` is memoized the exact same way on the base `PrestoBaseEngineSpec` (`presto.py:571`), so Trino already cached this identically via inheritance before this PR. The override just mirrors it (and honestly the memoize only got added because CodeAnt earlier flagged the repeated `get_indexes` call ;)).
It's partition metadata being cached here, not row data. If per-user isolation of that is genuinely a concern for OAuth2 connections, it'd be pre-existing and would hit Presto too, so better as its own issue than a blocker on this Iceberg fix.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41055:
URL: https://github.com/apache/superset/pull/41055#issuecomment-4770637198
<!-- translation-regression-bot -->
## ⚠️ Translation Regression Detected
A source change in this PR renamed or reworded strings, invalidating existing translations (they are now `#, fuzzy`) in `de`, `fi`, `th`. Please resolve the affected `.po` files before merging.
_Note: intentionally **deleting** a translatable string is not a regression and is not flagged here — only translations invalidated by a renamed/reworded source string are._
| Language | Fuzzy before | Fuzzy after | New |
|----------|-------------:|------------:|----:|
| `de` | 0 | 997 | +997 |
| `fi` | 4817 | 4823 | +6 |
| `th` | 4817 | 4823 | +6 |
### How to fix
**1. Install dependencies** (if not already set up):
```bash
pip install -r superset/translations/requirements.txt
sudo apt-get install gettext # or: brew install gettext
```
**2. Re-extract strings and sync `.po` files:**
```bash
./scripts/translations/babel_update.sh
```
This rewrites `superset/translations/messages.pot` from the current source files and merges the changes into every `.po` file. Strings whose `msgid` changed will be marked `#, fuzzy`.
**3. Resolve the fuzzy entries** in the affected language files (`de`, `fi`, `th`):
```bash
grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.po
```
For each fuzzy entry, either rewrite the `msgstr` to match the new string and remove the `#, fuzzy` line, or clear the `msgstr` to `""` if you cannot provide a translation.
**4. Commit your changes to the `.po` files.**
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "malon64 (via GitHub)" <gi...@apache.org>.
malon64 commented on PR #41055:
URL: https://github.com/apache/superset/pull/41055#issuecomment-4781370128
Alright if it's all good can I let you merge it @betodealmeida @rusackas ? I don't have the right sto do it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41055:
URL: https://github.com/apache/superset/pull/41055#discussion_r3414495245
##########
tests/unit_tests/db_engine_specs/test_trino.py:
##########
@@ -343,6 +343,124 @@ def test_get_extra_table_metadata(mocker: MockerFixture) -> None:
assert result["partitions"]["latest"] == {"ds": "01-01-19", "hour": 1}
+def test_get_extra_table_metadata_iceberg_unpartitioned(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Iceberg ``$partitions`` metadata fields must not be treated as partition
+ columns, so an unpartitioned Iceberg table yields no partition metadata and
+ no latest-partition query.
+ """
+ from superset.db_engine_specs.trino import TrinoEngineSpec
+
+ db_mock = mocker.MagicMock()
+ db_mock.get_indexes = Mock(
+ return_value=[
+ {
+ "name": "partition",
+ "column_names": ["data", "file_count", "record_count", "total_size"],
+ }
+ ]
+ )
+ db_mock.get_extra = Mock(return_value={})
+ db_mock.has_view = Mock(return_value=None)
+ db_mock.get_df = Mock()
+ result = TrinoEngineSpec.get_extra_table_metadata(
+ db_mock,
+ Table("test_table", "test_schema"),
+ )
+ assert "partitions" not in result
+ db_mock.get_df.assert_not_called()
+
+
+def test_get_extra_table_metadata_iceberg_partitioned(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A genuinely partitioned Iceberg table exposes a ``partition`` ROW column
+ alongside the metadata fields. Since the real partition columns are nested
+ in the ROW and not directly queryable here, the partition block is skipped
+ rather than generating an invalid latest-partition query.
+ """
+ from superset.db_engine_specs.trino import TrinoEngineSpec
+
+ db_mock = mocker.MagicMock()
+ db_mock.get_indexes = Mock(
+ return_value=[
+ {
+ "name": "partition",
+ "column_names": [
+ "partition",
+ "record_count",
+ "file_count",
+ "total_size",
+ "data",
+ ],
+ }
+ ]
+ )
+ db_mock.get_extra = Mock(return_value={})
+ db_mock.has_view = Mock(return_value=None)
+ db_mock.get_df = Mock()
+ result = TrinoEngineSpec.get_extra_table_metadata(
+ db_mock,
+ Table("test_table", "test_schema"),
+ )
+ assert "partitions" not in result
+ db_mock.get_df.assert_not_called()
+
+
+def test_filter_iceberg_partition_indexes() -> None:
+ from superset.db_engine_specs.trino import TrinoEngineSpec
Review Comment:
**Suggestion:** Add an inline docstring to this newly added test function so it complies with the requirement that new functions are documented. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added test function in the PR and it has no docstring immediately under the definition. The custom rule requires newly added Python functions and classes to include docstrings, so the suggestion correctly identifies a real violation.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=67b49aa201204f85b541560a24dce2d1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=67b49aa201204f85b541560a24dce2d1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/db_engine_specs/test_trino.py
**Line:** 413:414
**Comment:**
*Custom Rule: Add an inline docstring to this newly added test function so it complies with the requirement that new functions are documented.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41055&comment_hash=2cd36ead0245c95372e3c1d5869da7ba8f18a30169e032ad0856580af241dd52&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41055&comment_hash=2cd36ead0245c95372e3c1d5869da7ba8f18a30169e032ad0856580af241dd52&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #41055:
URL: https://github.com/apache/superset/pull/41055#issuecomment-4770661322
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/41055?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `34.37500%` with `21 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.59%. Comparing base ([`e5b6642`](https://app.codecov.io/gh/apache/superset/commit/e5b6642b186ec31fd48075a6bf97bc94023d8825?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`81a057e`](https://app.codecov.io/gh/apache/superset/commit/81a057ecbdf5575e07e22ce93ce09f0ab172a71c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 175 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/41055?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/db\_engine\_specs/trino.py](https://app.codecov.io/gh/apache/superset/pull/41055?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Ftrino.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3RyaW5vLnB5) | 39.28% | [17 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/db\_engine\_specs/presto.py](https://app.codecov.io/gh/apache/superset/pull/41055?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Fpresto.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (e5b6642) and HEAD (81a057e). Click for more details.
>
> <details><summary>HEAD has 17 uploads less than BASE</summary>
>
>| Flag | BASE (e5b6642) | HEAD (81a057e) |
>|------|------|------|
>|hive|3|1|
>|python|15|2|
>|presto|3|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #41055 +/- ##
==========================================
- Coverage 64.28% 55.59% -8.69%
==========================================
Files 2660 2660
Lines 144697 144724 +27
Branches 33366 33372 +6
==========================================
- Hits 93012 80455 -12557
- Misses 50037 63544 +13507
+ Partials 1648 725 -923
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/41055/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/41055/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.36% <34.37%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/41055/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/41055/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/41055/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.94% <34.37%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/41055/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.01% <34.37%> (-18.63%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/41055/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/41055/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/41055?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41055:
URL: https://github.com/apache/superset/pull/41055#discussion_r3454046199
##########
superset/db_engine_specs/trino.py:
##########
@@ -631,3 +680,66 @@ def get_indexes(
return super().get_indexes(database, inspector, table)
except NoSuchTableError:
return []
+
+ @classmethod
+ @cache_manager.data_cache.memoize(timeout=60)
+ def latest_partition(
+ cls,
+ database: Database,
+ table: Table,
+ show_first: bool = False,
+ indexes: list[dict[str, Any]] | None = None,
Review Comment:
**Suggestion:** This method is memoized in the global data cache even though Trino supports OAuth2 personal tokens, so cached latest-partition results can be reused across different users hitting the same database/table and expose user-scoped metadata incorrectly. Use a per-user cache key (or bypass shared memoization for OAuth2 connections) so partition metadata is isolated by user identity. [cache]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Trino Jinja `latest_partition` macro reuses partitions across users.
- ❌ Trino `where_latest_partition` filters share first user's partitions.
- ⚠️ Database `select_star` metadata can reflect another user's view.
- ⚠️ Violates per-user caching expectations for OAuth2 Trino connections.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Trino database with OAuth2 personal tokens so
`Database.is_oauth2_enabled()` is true and user-scoped tokens are used when creating
engines via `Database._get_sqla_engine` (see `superset/models/core.py:116-123`, which
calls `get_oauth2_access_token(...)` and passes the token into
`db_engine_spec.impersonate_user`).
2. From SQL Lab or a chart, run a query against this Trino database that uses the Jinja
partition helpers, for example `{{ latest_partition('schema.table') }}`, which is
implemented in `superset/jinja_context.py:996-1033` as
`latest_partitions()`/`first_latest_partition()` and calls
`self._database.db_engine_spec.latest_partition(database=self._database,
table=Table(table_name, schema))`.
3. For a Trino-backed database, `self._database.db_engine_spec` is `TrinoEngineSpec`,
whose `latest_partition` override at `superset/db_engine_specs/trino.py:684-693` is
decorated with `@cache_manager.data_cache.memoize(timeout=60)` using the global
`data_cache` from `superset/utils/cache_manager.py:187-214`. The first user (user A) to
hit this macro causes `TrinoEngineSpec.latest_partition` to execute, which delegates to
`PrestoBaseEngineSpec.latest_partition` in `superset/db_engine_specs/presto.py:568-647` to
run a SQL query via `database.get_df(...)` under user A's OAuth2 token, and the
`(col_names, latest_parts)` tuple is stored in `data_cache` keyed only on the method
arguments `(cls, database, table, show_first, indexes)`.
4. Within the 60-second memoization window, a second user (user B) with different
permissions on the same Trino database/table issues a query that triggers the same
`latest_partition` call (same `database`/`table` arguments) via either the same Jinja
macro (`superset/jinja_context.py:996-1033`) or `where_latest_partition` used by
`select_star` (see `superset/db_engine_specs/presto.py:520-559` calling
`cls.latest_partition` and `superset/db_engine_specs/base.py:41-97`), and the decorator at
`superset/db_engine_specs/trino.py:684-691` returns the cached result from user A without
re-executing `database.get_df` under user B's token; because `data_cache` keys do not
incorporate any impersonation key or `g.user`, user B receives partition metadata computed
under user A's OAuth2-scoped privileges, leaking user-scoped metadata across users.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6b852786d6194fc6897b1965ec6c9975&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6b852786d6194fc6897b1965ec6c9975&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/trino.py
**Line:** 684:691
**Comment:**
*Cache: This method is memoized in the global data cache even though Trino supports OAuth2 personal tokens, so cached latest-partition results can be reused across different users hitting the same database/table and expose user-scoped metadata incorrectly. Use a per-user cache key (or bypass shared memoization for OAuth2 connections) so partition metadata is isolated by user identity.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41055&comment_hash=f7e546d13d5950c974d9cfed29563b04b1957a8397f383800e6053009163e4af&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41055&comment_hash=f7e546d13d5950c974d9cfed29563b04b1957a8397f383800e6053009163e4af&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #41055:
URL: https://github.com/apache/superset/pull/41055
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "malon64 (via GitHub)" <gi...@apache.org>.
malon64 commented on code in PR #41055:
URL: https://github.com/apache/superset/pull/41055#discussion_r3414527727
##########
tests/unit_tests/db_engine_specs/test_trino.py:
##########
@@ -343,6 +343,124 @@ def test_get_extra_table_metadata(mocker: MockerFixture) -> None:
assert result["partitions"]["latest"] == {"ds": "01-01-19", "hour": 1}
+def test_get_extra_table_metadata_iceberg_unpartitioned(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Iceberg ``$partitions`` metadata fields must not be treated as partition
+ columns, so an unpartitioned Iceberg table yields no partition metadata and
+ no latest-partition query.
+ """
+ from superset.db_engine_specs.trino import TrinoEngineSpec
+
+ db_mock = mocker.MagicMock()
+ db_mock.get_indexes = Mock(
+ return_value=[
+ {
+ "name": "partition",
+ "column_names": ["data", "file_count", "record_count", "total_size"],
+ }
+ ]
+ )
+ db_mock.get_extra = Mock(return_value={})
+ db_mock.has_view = Mock(return_value=None)
+ db_mock.get_df = Mock()
+ result = TrinoEngineSpec.get_extra_table_metadata(
+ db_mock,
+ Table("test_table", "test_schema"),
+ )
+ assert "partitions" not in result
+ db_mock.get_df.assert_not_called()
+
+
+def test_get_extra_table_metadata_iceberg_partitioned(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A genuinely partitioned Iceberg table exposes a ``partition`` ROW column
+ alongside the metadata fields. Since the real partition columns are nested
+ in the ROW and not directly queryable here, the partition block is skipped
+ rather than generating an invalid latest-partition query.
+ """
+ from superset.db_engine_specs.trino import TrinoEngineSpec
+
+ db_mock = mocker.MagicMock()
+ db_mock.get_indexes = Mock(
+ return_value=[
+ {
+ "name": "partition",
+ "column_names": [
+ "partition",
+ "record_count",
+ "file_count",
+ "total_size",
+ "data",
+ ],
+ }
+ ]
+ )
+ db_mock.get_extra = Mock(return_value={})
+ db_mock.has_view = Mock(return_value=None)
+ db_mock.get_df = Mock()
+ result = TrinoEngineSpec.get_extra_table_metadata(
+ db_mock,
+ Table("test_table", "test_schema"),
+ )
+ assert "partitions" not in result
+ db_mock.get_df.assert_not_called()
+
+
+def test_filter_iceberg_partition_indexes() -> None:
+ from superset.db_engine_specs.trino import TrinoEngineSpec
Review Comment:
docstring added
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41055:
URL: https://github.com/apache/superset/pull/41055#issuecomment-4709323916
<!-- Bito Reply -->
The review suggestion to add a docstring to the newly added test function `test_filter_iceberg_partition_indexes` is correct, as it currently lacks one. You can resolve this by adding a docstring immediately after the function definition.
Would you like me to implement this fix for you? I can also check the rest of the PR for similar issues if you would like me to proceed with a broader review.
**tests/unit_tests/db_engine_specs/test_trino.py**
```
def test_filter_iceberg_partition_indexes() -> None:
"""Test that Iceberg partition indexes are correctly filtered."""
from superset.db_engine_specs.trino import TrinoEngineSpec
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "malon64 (via GitHub)" <gi...@apache.org>.
malon64 commented on code in PR #41055:
URL: https://github.com/apache/superset/pull/41055#discussion_r3414642363
##########
superset/db_engine_specs/trino.py:
##########
@@ -631,3 +680,34 @@ def get_indexes(
return super().get_indexes(database, inspector, table)
except NoSuchTableError:
return []
+
+ @classmethod
+ def latest_partition(
+ cls,
+ database: Database,
+ table: Table,
+ show_first: bool = False,
+ indexes: list[dict[str, Any]] | None = None,
+ ) -> tuple[list[str], list[str] | None]:
+ """
+ Return the latest partition for a table.
+
+ Iceberg "$partitions" metadata fields are filtered out first, so we
+ never build a latest-partition query against them.
+
+ :param database: the database the query will be run against
+ :param table: the table instance
+ :param show_first: return the value for the first partitioning key when
+ there are several
+ :param indexes: the indexes associated with the table
+ :returns: the column names and the latest partition values
+ """
+ if indexes is None:
+ indexes = database.get_indexes(table)
+
+ return super().latest_partition(
+ database,
+ table,
+ show_first=show_first,
+ indexes=cls._filter_iceberg_partition_indexes(indexes),
+ )
Review Comment:
Added an optional `indexes` arg to PrestoBaseEngineSpec.latest_sub_partition
(mirroring latest_partition) plus an empty-indexes guard, and overrode it in
TrinoEngineSpec to pass _filter_iceberg_partition_indexes(). All partition
paths — get_extra_table_metadata, latest_partition, latest_sub_partition,
and where_latest_partition — now exclude the metadata fields consistently.
Added a regression test (test_latest_sub_partition_iceberg).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41055:
URL: https://github.com/apache/superset/pull/41055#issuecomment-4710966273
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #41055:
URL: https://github.com/apache/superset/pull/41055#issuecomment-4771021351
@betodealmeida this looks good to me, but please feel free to provide a second opinion, as SQL guru ;)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41055:
URL: https://github.com/apache/superset/pull/41055#discussion_r3414514581
##########
superset/db_engine_specs/trino.py:
##########
@@ -631,3 +680,34 @@ def get_indexes(
return super().get_indexes(database, inspector, table)
except NoSuchTableError:
return []
+
+ @classmethod
+ def latest_partition(
+ cls,
+ database: Database,
+ table: Table,
+ show_first: bool = False,
+ indexes: list[dict[str, Any]] | None = None,
+ ) -> tuple[list[str], list[str] | None]:
+ """
+ Return the latest partition for a table.
+
+ Iceberg "$partitions" metadata fields are filtered out first, so we
+ never build a latest-partition query against them.
+
+ :param database: the database the query will be run against
+ :param table: the table instance
+ :param show_first: return the value for the first partitioning key when
+ there are several
+ :param indexes: the indexes associated with the table
+ :returns: the column names and the latest partition values
+ """
+ if indexes is None:
+ indexes = database.get_indexes(table)
+
+ return super().latest_partition(
+ database,
+ table,
+ show_first=show_first,
+ indexes=cls._filter_iceberg_partition_indexes(indexes),
+ )
Review Comment:
**🟠 Architect Review — HIGH**
TrinoEngineSpec only filters Iceberg `$partitions` metadata in latest_partition; latest_sub_partition is inherited unchanged from PrestoBaseEngineSpec and still uses raw database.get_indexes(table), so the trino.latest_sub_partition() Jinja macro can build partition SQL against Iceberg `$partitions` metadata columns instead of real partition keys.
**Suggestion:** Override latest_sub_partition in TrinoEngineSpec (or otherwise ensure it uses _filter_iceberg_partition_indexes) so that all Trino partition macros, including trino.latest_sub_partition(), exclude Iceberg `$partitions` metadata fields consistently.
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a9f6e4f947b7459ba3d6f263bbf5e21c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a9f6e4f947b7459ba3d6f263bbf5e21c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset/db_engine_specs/trino.py
**Line:** 685:713
**Comment:**
*HIGH: TrinoEngineSpec only filters Iceberg `$partitions` metadata in latest_partition; latest_sub_partition is inherited unchanged from PrestoBaseEngineSpec and still uses raw database.get_indexes(table), so the trino.latest_sub_partition() Jinja macro can build partition SQL against Iceberg `$partitions` metadata columns instead of real partition keys.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41055:
URL: https://github.com/apache/superset/pull/41055#issuecomment-4772387531
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #41055:
URL: https://github.com/apache/superset/pull/41055#issuecomment-4770729889
Thanks @malon64 - really solid take on a bug that's been biting folks for too long!
I traced the preview path through, and the part that actually matters (where_latest_partition falling through to the filtered latest_partition) is covered, so previews should stop injecting those bogus record_count/file_count WHERE clauses. The two-condition gate so we don't clobber a Hive table that happens to have a record_count column is a nice touch too.
Only thing in the way is CI - babel-extract is red and the bot's flagging ~997 fuzzy de strings, which is odd for a backend-only change that doesn't touch any translatable strings. Pretty sure that's just a stale branch... I'll see if I can update it now to see if that helps it, but otherwise happy to approve.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #41055:
URL: https://github.com/apache/superset/pull/41055#issuecomment-4781668254
Thanks @malon64!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): ignore Iceberg $partitions metadata fields in partition detection [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #41055:
URL: https://github.com/apache/superset/pull/41055#discussion_r3461269403
##########
superset/db_engine_specs/trino.py:
##########
@@ -631,3 +680,66 @@ def get_indexes(
return super().get_indexes(database, inspector, table)
except NoSuchTableError:
return []
+
+ @classmethod
+ @cache_manager.data_cache.memoize(timeout=60)
+ def latest_partition(
+ cls,
+ database: Database,
+ table: Table,
+ show_first: bool = False,
+ indexes: list[dict[str, Any]] | None = None,
Review Comment:
Yeah, it's Ok to cache latest partition without the user key.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org