fix: unaligned metric signature#27370
Conversation
github-actions
Bot
added
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Aligns MariaDB and SingleStore profiler percentile/window metric overrides with the base metric call signature to prevent runtime TypeError during metric computation (fixes #27207 in the ingestion profiler).
Changes:
- Updated MariaDB and SingleStore median/first_quartile/third_quartile
_compute_sqa_fnoverrides to accept the optionaldimension_colargument. - Added a unit test suite to assert database-specific metric overrides remain signature-compatible with their parents.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| ingestion/src/metadata/profiler/source/database/mariadb/metrics/window/median.py | Align MariaDB median override signature with base metric call-site. |
| ingestion/src/metadata/profiler/source/database/mariadb/metrics/window/first_quartile.py | Align MariaDB first quartile override signature. |
| ingestion/src/metadata/profiler/source/database/mariadb/metrics/window/third_quartile.py | Align MariaDB third quartile override signature. |
| ingestion/src/metadata/profiler/source/database/single_store/metrics/window/median.py | Align SingleStore median override signature with base metric call-site. |
| ingestion/src/metadata/profiler/source/database/single_store/metrics/window/first_quartile.py | Align SingleStore first quartile override signature. |
| ingestion/src/metadata/profiler/source/database/single_store/metrics/window/third_quartile.py | Align SingleStore third quartile override signature. |
| ingestion/tests/unit/observability/profiler/sqlalchemy/test_inherited_metrics/test_metric_signatures.py | New unit tests guarding signature compatibility across DB-specific overrides. |
| ingestion/tests/unit/observability/profiler/sqlalchemy/test_inherited_metrics/init.py | Adds test package init for the new test module. |
|
|
||
| class MariaDBMedian(Median): | ||
| def _compute_sqa_fn(self, column, table, percentile): | ||
| def _compute_sqa_fn(self, column, table, percentile, dimension_col=None): |
There was a problem hiding this comment.
dimension_col is now part of the override signature but it is unused in the function body, which will trigger unused-argument in pylint (W0613) for this file. Consider either incorporating dimension_col into the returned function expression (e.g., pass it through when not None) or explicitly disabling the warning for this method if the argument is intentionally ignored.
| def _compute_sqa_fn(self, column, table, percentile, dimension_col=None): | |
| def _compute_sqa_fn( # pylint: disable=unused-argument | |
| self, column, table, percentile, dimension_col=None | |
| ): |
| class MariaDBThirdQuartile(ThirdQuartile): | ||
| def _compute_sqa_fn(self, column, table, percentile): | ||
| def _compute_sqa_fn(self, column, table, percentile, dimension_col=None): | ||
| """Generic method to compute the quartile using sqlalchemy""" |
There was a problem hiding this comment.
dimension_col is now part of the override signature but it is unused in the function body, which will trigger unused-argument in pylint (W0613) for this file. Consider either incorporating dimension_col into the returned function expression (e.g., pass it through when not None) or explicitly disabling the warning for this method if the argument is intentionally ignored.
| """Generic method to compute the quartile using sqlalchemy""" | |
| """Generic method to compute the quartile using sqlalchemy""" | |
| _ = dimension_col |
|
|
||
| class SingleStoreMedian(Median): | ||
| def _compute_sqa_fn(self, column, table, percentile): | ||
| def _compute_sqa_fn(self, column, table, percentile, dimension_col=None): |
There was a problem hiding this comment.
dimension_col is now part of the override signature but it is unused in the function body, which will trigger unused-argument in pylint (W0613) for this file. Consider either incorporating dimension_col into the returned function expression (e.g., pass it through when not None) or explicitly disabling the warning for this method if the argument is intentionally ignored.
| def _compute_sqa_fn(self, column, table, percentile, dimension_col=None): | |
| def _compute_sqa_fn( # pylint: disable=unused-argument | |
| self, column, table, percentile, dimension_col=None | |
| ): |
| class SingleStoreFirstQuartile(FirstQuartile): | ||
| def _compute_sqa_fn(self, column, table, percentile): | ||
| def _compute_sqa_fn(self, column, table, percentile, dimension_col=None): | ||
| """Generic method to compute the quartile using sqlalchemy""" |
There was a problem hiding this comment.
dimension_col is now part of the override signature but it is unused in the function body, which will trigger unused-argument in pylint (W0613) for this file. Consider either incorporating dimension_col into the returned function expression (e.g., pass it through when not None) or explicitly disabling the warning for this method if the argument is intentionally ignored.
| """Generic method to compute the quartile using sqlalchemy""" | |
| """Generic method to compute the quartile using sqlalchemy""" | |
| _ = dimension_col |
| class SingleStoreThirdQuartile(ThirdQuartile): | ||
| def _compute_sqa_fn(self, column, table, percentile): | ||
| def _compute_sqa_fn(self, column, table, percentile, dimension_col=None): | ||
| """Generic method to compute the quartile using sqlalchemy""" |
There was a problem hiding this comment.
dimension_col is now part of the override signature but it is unused in the function body, which will trigger unused-argument in pylint (W0613) for this file. Consider either incorporating dimension_col into the returned function expression (e.g., pass it through when not None) or explicitly disabling the warning for this method if the argument is intentionally ignored.
| """Generic method to compute the quartile using sqlalchemy""" | |
| """Generic method to compute the quartile using sqlalchemy""" | |
| _ = dimension_col |
| def test_compute_sqa_fn_accepts_parent_args(child_cls): | ||
| """Child _compute_sqa_fn must accept all arguments the parent fn() passes. | ||
|
|
||
| The parent Median.fn() passes dimension_col as a keyword argument. |
There was a problem hiding this comment.
The docstring says Median.fn() passes dimension_col as a keyword argument, but the implementation currently passes it as a 4th positional argument. Please update the comment to match the actual call-site so the test intent/documentation stays accurate.
| The parent Median.fn() passes dimension_col as a keyword argument. | |
| The parent Median.fn() passes dimension_col as the 4th positional argument. |
🔴 Playwright Results — 2 failure(s), 19 flaky✅ 3664 passed · ❌ 2 failed · 🟡 19 flaky · ⏭️ 89 skipped
Genuine Failures (failed on all attempts)❌
|
| @pytest.fixture(scope="module") | ||
| def singlestore_engine(): | ||
| container = ( | ||
| DockerContainer(image="ghcr.io/singlestore-labs/singlestoredb-dev:latest") |
There was a problem hiding this comment.
The SingleStore testcontainer uses an image tag :latest, which is non-deterministic and can break tests when the upstream image changes. Pin to a specific SingleStore image version to keep CI stable and reproducible.
| DockerContainer(image="ghcr.io/singlestore-labs/singlestoredb-dev:latest") | |
| DockerContainer(image="ghcr.io/singlestore-labs/singlestoredb-dev:8.5.22") |
| percentile = clauses[2].value | ||
| dimension_col = clauses[3].value if len(clauses) > 3 else None | ||
| if dimension_col: | ||
| return ( | ||
| f"(SELECT approx_percentile({col}, {percentile:.2f}) " | ||
| f"FROM {table} AS median_inner " | ||
| f"WHERE median_inner.{dimension_col} = {table}.{dimension_col})" |
There was a problem hiding this comment.
In the correlated-path SQL, the table name is interpolated without any quoting/escaping. This can break for tables that require quoting (reserved words, special chars) and is inconsistent with the MySQL MedianFn compiler which quotes table names. Consider using the dialect preparer to quote the table identifier consistently in both the FROM and correlated WHERE references.
| percentile = clauses[2].value | |
| dimension_col = clauses[3].value if len(clauses) > 3 else None | |
| if dimension_col: | |
| return ( | |
| f"(SELECT approx_percentile({col}, {percentile:.2f}) " | |
| f"FROM {table} AS median_inner " | |
| f"WHERE median_inner.{dimension_col} = {table}.{dimension_col})" | |
| quoted_table = compiler.preparer.quote(table) | |
| percentile = clauses[2].value | |
| dimension_col = clauses[3].value if len(clauses) > 3 else None | |
| if dimension_col: | |
| return ( | |
| f"(SELECT approx_percentile({col}, {percentile:.2f}) " | |
| f"FROM {quoted_table} AS median_inner " | |
| f"WHERE median_inner.{dimension_col} = {quoted_table}.{dimension_col})" |
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
Code Review ✅ Approved 1 resolved / 1 findingsRestored the dimension_col in the metric signature, addressing the silent data drop and ensuring correct dimension grouping. ✅ 1 resolved✅ Bug: dimension_col silently dropped — dimension grouping won't work
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
|
Changes have been cherry-picked to the 1.12.6 branch. |
* fix: unaligned metric signature * fix: added dimension imp. for mariadb and singlestore + integration tests * fix: ci failure * fix: ci failure * fix: ci failure * fix: ci failures (cherry picked from commit 5204148)
Describe your changes:
Fixes #27207
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>