Conversation
…case catalog matching HANA's SYS.M_TABLES lacks CREATE_TIME — query SYS.TABLES via CTE + LEFT JOIN. HANA catalog stores identifiers uppercase but OM stores lowercase — apply .upper() to WHERE filters.
github-actions
Bot
added
Ingestion
safe to test
There was a problem hiding this comment.
Pull request overview
Fixes SAP HANA table profiling so createDateTime is correctly populated and table/schema matching works when OpenMetadata stores identifiers in lowercase but HANA system views use uppercase.
Changes:
- Update
SAPHanaTableMetricComputerto pullCREATE_TIMEfromSYS.TABLESvia a second CTE andLEFT JOINtoSYS.M_TABLES. - Uppercase schema/table identifiers used in the HANA system-view filters to ensure rows match.
- Add unit tests covering the new CTE/join structure, uppercase matching, and
NULLcreate-time behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
ingestion/src/metadata/profiler/orm/functions/table_metric_computer.py |
Reworks HANA metrics query to join SYS.M_TABLES with SYS.TABLES and uppercase identifiers for matching. |
ingestion/tests/unit/observability/profiler/test_table_metric_computer.py |
Adds tests validating the new HANA query shape (CTEs + LEFT JOIN) and uppercase filtering behavior. |
| assert sql_upper.count("WITH") >= 1 or sql_upper.count("AS (") >= 2 or sql_upper.count("AS \n(") >= 2, ( | ||
| f"Expected two CTEs in query, got: {sql}" | ||
| ) |
There was a problem hiding this comment.
The assertion in this test doesn’t actually verify that there are two CTEs: sql_upper.count("WITH") >= 1 will pass even with a single CTE, and the or chain makes the check very permissive. Consider tightening this to assert the presence of both CTE definitions (e.g., both M_TABLES and TABLES appear in the WITH ... clause, or count CTE AS ( occurrences in a dialect-stable way).
| assert sql_upper.count("WITH") >= 1 or sql_upper.count("AS (") >= 2 or sql_upper.count("AS \n(") >= 2, ( | |
| f"Expected two CTEs in query, got: {sql}" | |
| ) | |
| assert "WITH" in sql_upper, f"Expected WITH clause in query, got: {sql}" | |
| assert "M_TABLES AS (" in sql_upper, ( | |
| f"Expected M_TABLES CTE definition in query, got: {sql}" | |
| ) | |
| assert "TABLES AS (" in sql_upper, ( | |
| f"Expected TABLES CTE definition in query, got: {sql}" | |
| ) |
| """When table not found in either system view, execute().first() returns None.""" | ||
| session = _build_mock_session() | ||
| session.execute.return_value.first.return_value = None | ||
| computer = _build_computer(session, SAPHanaTableMetricComputer) | ||
| result = computer.compute() | ||
| assert result is None |
There was a problem hiding this comment.
test_compute_returns_none_for_nonexistent_table duplicates test_compute_returns_none_when_no_result earlier in the same class (both only assert compute() returns None when execute().first() is None). Consider removing one, or adjusting this one to cover a distinct case not already exercised.
| """When table not found in either system view, execute().first() returns None.""" | |
| session = _build_mock_session() | |
| session.execute.return_value.first.return_value = None | |
| computer = _build_computer(session, SAPHanaTableMetricComputer) | |
| result = computer.compute() | |
| assert result is None | |
| """When table is absent from HANA system views, compute returns None and | |
| still queries using uppercased identifiers expected by the catalog.""" | |
| session = _build_mock_session() | |
| session.execute.return_value.first.return_value = None | |
| computer = _build_computer(session, SAPHanaTableMetricComputer) | |
| result = computer.compute() | |
| sql = str( | |
| session.execute.call_args[0][0].compile( | |
| compile_kwargs={"literal_binds": True} | |
| ) | |
| ) | |
| assert result is None | |
| assert "TEST_SCHEMA" in sql, ( | |
| f"Nonexistent-table lookup must use uppercased schema name, got: {sql}" | |
| ) | |
| assert "TEST_TABLE" in sql, ( | |
| f"Nonexistent-table lookup must use uppercased table name, got: {sql}" | |
| ) |
| session.execute.return_value.first.return_value = mock_result | ||
| computer = _build_computer(session, SAPHanaTableMetricComputer) | ||
| computer.compute() | ||
| sql = str(session.execute.call_args[0][0].compile(compile_kwargs={"literal_binds": True})) |
There was a problem hiding this comment.
These .compile(compile_kwargs={"literal_binds": True}) lines exceed Black’s default line length (88) and will likely fail the repo’s black --check formatting step (see ingestion/noxfile.py:55). Running Black should wrap these expressions into multiple lines.
| sql = str(session.execute.call_args[0][0].compile(compile_kwargs={"literal_binds": True})) | |
| sql = str( | |
| session.execute.call_args[0][0].compile( | |
| compile_kwargs={"literal_binds": True} | |
| ) | |
| ) |
| session.execute.return_value.first.return_value = mock_result | ||
| computer = _build_computer(session, SAPHanaTableMetricComputer) | ||
| computer.compute() | ||
| sql = str(session.execute.call_args[0][0].compile(compile_kwargs={"literal_binds": True})) |
There was a problem hiding this comment.
This line is likely not Black-formatted (it exceeds Black’s 88-char line length). Please run the formatter or wrap the str(session.execute...compile(...)) call so black --check passes.
| session.execute.return_value.first.return_value = mock_result | ||
| computer = _build_computer(session, SAPHanaTableMetricComputer) | ||
| computer.compute() | ||
| sql = str(session.execute.call_args[0][0].compile(compile_kwargs={"literal_binds": True})) |
There was a problem hiding this comment.
This line likely exceeds Black’s 88-character limit and will fail formatting checks. Consider wrapping the .compile(compile_kwargs={...}) call across lines and re-running black.
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
…ack formatting - Guard against None schema/table name before .upper() in HANA compute - Tighten CTE assertion to check both M_TABLES and TABLES definitions - Make nonexistent-table test distinct by verifying uppercase in query - Apply Black formatting fixes
Agent-Logs-Url: https://github.com/open-metadata/OpenMetadata/sessions/175f9aa0-30e5-49a2-84af-d4e76fb01a14 Co-authored-by: IceS2 <4912399+IceS2@users.noreply.github.com>
| "Missing schema or table name for HANA table metric computation" | ||
| ) | ||
| return None |
There was a problem hiding this comment.
The early guard returns None when schema_name/table_name are missing. Elsewhere in this module, missing names fall back to the base computation (see TableMetricComputerFactory.construct() fallback behavior), so returning None here can silently skip profiling. Consider falling back to super().compute() (or raising to trigger factory fallback) and include the actual schema/table values in the log message for debugging.
| "Missing schema or table name for HANA table metric computation" | |
| ) | |
| return None | |
| "Missing schema or table name for HANA table metric computation. " | |
| "Falling back to base computation with schema_name=%r, table_name=%r", | |
| self.schema_name, | |
| self.table_name, | |
| ) | |
| return super().compute() |
…ssing names - Restore docstring on HANA compute() for consistency with other computers - Fall back to super().compute() instead of None when schema/table name missing - Include schema/table values in warning log for debugging
Code Review ✅ ApprovedCorrects SAP HANA metadata retrieval by mapping CREATE_TIME from SYS.TABLES and normalizing catalog object matching to uppercase. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
pmbrull
deleted the
task/debug-sap-hana-profiler-create-time-miss-53888850
branch
…og matching (#27483) * fix(profiler): SAP HANA CREATE_TIME sourced from SYS.TABLES and uppercase catalog matching HANA's SYS.M_TABLES lacks CREATE_TIME — query SYS.TABLES via CTE + LEFT JOIN. HANA catalog stores identifiers uppercase but OM stores lowercase — apply .upper() to WHERE filters. * fix checkstyle * fix(profiler): address PR review — null guard, tighter assertions, Black formatting - Guard against None schema/table name before .upper() in HANA compute - Tighten CTE assertion to check both M_TABLES and TABLES definitions - Make nonexistent-table test distinct by verifying uppercase in query - Apply Black formatting fixes * fix(profiler): tighten HANA CTE assertions and restore compute docstring Agent-Logs-Url: https://github.com/open-metadata/OpenMetadata/sessions/175f9aa0-30e5-49a2-84af-d4e76fb01a14 Co-authored-by: IceS2 <4912399+IceS2@users.noreply.github.com> * fix(profiler): normalize HANA SQL assertions for stable CTE checks Agent-Logs-Url: https://github.com/open-metadata/OpenMetadata/sessions/175f9aa0-30e5-49a2-84af-d4e76fb01a14 Co-authored-by: IceS2 <4912399+IceS2@users.noreply.github.com> * fix(profiler): add docstring and fallback to super().compute() for missing names - Restore docstring on HANA compute() for consistency with other computers - Fall back to super().compute() instead of None when schema/table name missing - Include schema/table values in warning log for debugging --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Fixes #27439
Summary
SYS.M_TABLESlacksCREATE_TIME. QuerySYS.TABLESvia a second CTE + LEFT JOIN socreateDateTimeis populated in table profiles..upper()to schema/table names in WHERE clauses so rows actually match.Test plan
pytest test_table_metric_computer.py— 28/28)libraries_table profiled with rowCount=7, columnCount=9, sizeInByte=16480, createDateTime=2026-04-16T15:32:08