Skip to content

feat(fabric): Ensure TIMESTAMPTZ is used with AT TIME ZONE#5362

Merged
georgesittas merged 3 commits intotobymao:mainfrom
mattiasthalen:fix/fabric-datetimeoffset
Jul 7, 2025
Merged

feat(fabric): Ensure TIMESTAMPTZ is used with AT TIME ZONE#5362
georgesittas merged 3 commits intotobymao:mainfrom
mattiasthalen:fix/fabric-datetimeoffset

Conversation

@mattiasthalen
Copy link
Contributor

@mattiasthalen mattiasthalen commented Jul 7, 2025

This pull request updates the handling of TIMESTAMPTZ and DATETIMEOFFSET types in the Fabric SQL dialect to ensure compliance with Microsoft Fabric's requirements. It introduces logic to differentiate how TIMESTAMPTZ is treated based on whether it is used in an AT TIME ZONE expression and adjusts test cases accordingly.

Changes to TIMESTAMPTZ and DATETIMEOFFSET handling:

  • Logic for TIMESTAMPTZ in datatype_sql:
    • Added a check to wrap TIMESTAMPTZ in AT TIME ZONE 'UTC' when AT TIME ZONE is missing.

Updates to test cases:

  • Modified precision handling for DATETIMEOFFSET:

    • Adjusted tests to ensure DATETIMEOFFSET is only valid when paired with AT TIME ZONE, preserving precision limits.
  • Added new test for TIMESTAMPTZ behavior:

    • Introduced a dedicated test to verify that TIMESTAMPTZ is used with AT TIME ZONE.

Copy link
Collaborator

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @mattiasthalen!

@georgesittas
Copy link
Collaborator

Is there something else pending for this PR on your end? Looks good to go.

@mattiasthalen
Copy link
Contributor Author

Is there something else pending for this PR on your end? Looks good to go.

Yeah, but I'm not sure if the error is here or in SQLMesh 😄
I'm trying to handle the fabric integration tests, and prior to your suggestion I got this error:

FAILED tests/core/engine_adapter/integration/test_integration.py::test_to_time_column[fabric-2020-01-01 00:00:00+00:00-time_column_type2-None-result2] - AssertionError: assert Timestamp('2020-01-01 00:00:00') == Timestamp('2020-01-01 00:00:00+0000', tz='UTC')

But now it's back to:

FAILED tests/core/engine_adapter/integration/test_integration.py::test_to_time_column[fabric-2020-01-01 00:00:00+00:00-time_column_type2-None-result2] - pyodbc.ProgrammingError: ('ODBC SQL type -155 is not yet supported.  column-index=0  type=...

But I'm struggling in general to see the query actually executed.

@georgesittas
Copy link
Collaborator

georgesittas commented Jul 7, 2025

Can you see the queries executed under logs/? Also consider using the --debug flag, or manually call configure_logging to log the queries for the integration test, when ran with pytest.

@mattiasthalen
Copy link
Contributor Author

Can you see the queries executed under logs/? Also consider using the --debug flag, or manually call configure_logging to log the queries for the integration test, when ran with pytest.

I think I've narrowed the issue down.
What happens is that when it recevies TIMESTAMPTZ without a AT TIME ZONE it truncates the TZ info and changes to TIMESTAMP. What needs to happen (I think) is to cast it to UTC.

@georgesittas
Copy link
Collaborator

@mattiasthalen let me know when this is ready to be merged.

@mattiasthalen
Copy link
Contributor Author

Will mark ready ASAP, just want to check a couple of things ☺️

@mattiasthalen mattiasthalen force-pushed the fix/fabric-datetimeoffset branch from 5dffef6 to 07e75f7 Compare July 7, 2025 16:30
@mattiasthalen mattiasthalen changed the title feat(fabric): Treat TIMESTAMPTZ as TIMESTAMP if not used with AT TIME ZONE feat(fabric): Ensure TIMESTAMPTZ is used with AT TIME ZONE Jul 7, 2025
@mattiasthalen mattiasthalen marked this pull request as ready for review July 7, 2025 16:40
@georgesittas georgesittas merged commit 1fd757e into tobymao:main Jul 7, 2025
7 checks passed
@mattiasthalen mattiasthalen deleted the fix/fabric-datetimeoffset branch July 7, 2025 16:46
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