Skip to content

Conversation

@vmatt
Copy link

@vmatt vmatt commented Oct 26, 2025

This PR improves Oracle database support by allowing custom ports to be defined, Presto and Trino timestamp normalization, optimizes Presto container configuration, and enhances test coverage across multiple databases.

Changes

Oracle Database Support

  • Added Oracle container to docker-compose with proper configuration
  • Implemented Oracle database connector with support for:
    • Custom port configuration (default: 1521)
    • ForeignKey type handling
    • Query parameter conversion from %s to :1, :2, ... format to support sqeleton query builder
  • Added Oracle credentials to dev environment configuration
  • Added Oracle connection string to test suite

Presto Improvements

  • Configuration optimization: Reduced memory requirements from 16GB to 8GB
    • Updated jvm.config: -Xmx16G-Xmx8G
    • Updated config.properties: Reduced query memory limits (5GB → 2GB, 1GB → 512MB, 2GB → 1GB)
    • Added JVM flag: -Djdk.attach.allowAttachSelf=true
  • Container setup: Replaced custom Dockerfile with official prestodb/presto:latest image
  • Volume mounting: Changed to explicit file mounting for better configuration control
  • Timestamp normalization: Fixed to use timestamp instead of timestamp(N) as Presto doesn't support precision in cast expressions

Trino Improvements

  • Timestamp normalization: Enhanced to properly use timestamp(N) precision in casts
  • Improved precision handling: Uses precision 6 (microseconds) for maximum precision when not rounding
  • Updated volume mounting to use explicit file mounts
  • Changed HTTPS port mapping from 8443 to 8444 to avoid conflicts

Test Infrastructure

  • Applied @test_each_database decorator to TestDatabase and TestMD5 test classes
  • Added comprehensive TestNormalizeValue test class with:
    • test_normalize_uuid: Tests UUID normalization across databases
    • test_normalize_timestamp: Tests timestamp normalization with different precisions
    • test_normalize_number: Tests numeric value normalization with various precisions
  • Enhanced TestThreePartIds to exclude DuckDB (doesn't support catalog-scoped information_schema queries)
  • Updated timezone handling in TestQueries to use UTC and handle Presto/Trino/Dremio precision limitations

Configuration Updates

  • Removed version field from docker-compose.yml (deprecated in newer Docker Compose versions)
  • Added oracle-volume to volumes section
  • Updated port mappings to avoid conflicts (Trino HTTPS: 8443 → 8444)
  • Added connection string examples in comments for Presto and Trino

Testing

All changes have been tested with the updated test suite. The new @test_each_database decorator ensures consistent testing across all supported databases.

Breaking Changes

None. All changes are backwards compatible.

TODO / Notes

  • Oracle database still requires cx_oracle deprecated package, might worth updating it to oracledb Python package

Comment on lines 201 to 208
# Convert %s style parameters to :1, :2, :3 style for Oracle to support queries built by sqeleton (tbl.create())
if sql_code.args:
# Replace %s with :1, :2, :3, etc.
oracle_sql = sql_code.code
for i in range(len(sql_code.args)):
oracle_sql = oracle_sql.replace('%s', f':{i+1}', 1)
sql_code = CompiledCode(oracle_sql, sql_code.args, sql_code.type)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erezsh - Would ARG_SYMBOL work here? I'm afraid that because parameters in oracle incrementing, it will not work with it.

Copy link
Owner

@erezsh erezsh Oct 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a quick glance, looks like the options are either to set ARG_SYMBOL = None, and use a custom implementation, or update eval_template() to support an incrementing ARG_SYMBOL, in addition to a constant one.

tests/common.py Outdated
TEST_BIGQUERY_CONN_STRING: str = os.environ.get("BIGQUERY_URI") or None
TEST_REDSHIFT_CONN_STRING: str = os.environ.get("REDSHIFT_URI") or None
TEST_ORACLE_CONN_STRING: str = None
TEST_ORACLE_CONN_STRING: str = "oracle://oracle:Password1@localhost/app" or None
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Oracle requires client libraries to be installed

@vmatt vmatt marked this pull request as draft October 26, 2025 02:43
@erezsh
Copy link
Owner

erezsh commented Oct 26, 2025

@vmatt Looks like this would be better as 3 or maybe even 4 separate Pull Requests, so that we can discuss each one in its own thread, and advance them separately.

@erezsh
Copy link
Owner

erezsh commented Oct 26, 2025

The new @test_each_database decorator ensures consistent testing across all supported databases.

Not sure what that means. Doesn't look like the decorator has changed at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants