fix(profiler): SAP HANA CREATE_TIME from SYS.TABLES + uppercase catalog matching#27483
Open
fix(profiler): SAP HANA CREATE_TIME from SYS.TABLES + uppercase catalog matching#27483
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
Contributor
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. |
Comment on lines
+353
to
+355
| 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).
Suggested change
| 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}" | |
| ) |
Comment on lines
+362
to
+367
| """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.
Suggested change
| """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.
Suggested change
| 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.
Contributor
|
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
TeddyCr
previously approved these changes
Apr 17, 2026
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>
auto-merge was automatically disabled
April 17, 2026 15:47
Head branch was pushed to by a user without write access
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>
Code Review ✅ ApprovedUpdates SAP HANA profiler to correctly parse CREATE_TIME from SYS.TABLES and implements case-insensitive catalog matching. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
Contributor
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
Contributor
🔴 Playwright Results — 3 failure(s), 26 flaky✅ 3634 passed · ❌ 3 failed · 🟡 26 flaky · ⏭️ 111 skipped
Genuine Failures (failed on all attempts)❌
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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