Skip to content

MySQL schema change to support fractional time #984

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

bpkroth
Copy link
Contributor

@bpkroth bpkroth commented Jun 2, 2025

Pull Request

Title

MySQL schema change to support fractional time


Description

  • Changes the storage schema to support a variant for MySQL that can handle fractional time.
  • Adds a custom column comparator to be able to pick that up by default in the future.
  • Adjusts tests to check for fractional time support.

Type of Change

  • 🛠️ Bug fix
  • ✨ New feature
  • 🔄 Refactor
  • 🧪 Tests

Testing

  • A lot of manual testing with docker.
  • Additional CI tests
  • Existing CI tests

Additional Notes (optional)

Part of a series of changes, should be merged after #983


bpkroth and others added 30 commits May 28, 2025 09:13
@Copilot Copilot AI review requested due to automatic review settings June 2, 2025 21:09
@bpkroth bpkroth requested a review from a team as a code owner June 2, 2025 21:09
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds support for fractional seconds in MySQL storage columns and aligns the test suite to use a new docker_hostname fixture.

  • Updated all relevant DateTime columns to use mysql.DATETIME(fsp=6) variants and removed microsecond truncation in trial creation.
  • Introduced a custom Alembic type comparator and schema-reset helpers for testing.
  • Refactored test fixtures/configs: replaced per-module SSH host fixtures with a shared docker_hostname, updated JSON configs, and adjusted telemetry tests for fractional timestamps.

Reviewed Changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/services/remote/ssh/test_ssh_host_service.py Consolidated imports to include wait_docker_service_socket.
tests/services/remote/ssh/fixtures.py Switched from ssh_test_server_hostname to shared docker_hostname.
tests/services/remote/ssh/conftest.py Removed obsolete hostname fixture alias.
tests/services/remote/ssh/init.py Dropped duplicate helper and plugin imports.
tests/environments/remote/conftest.py Removed unused hostname fixture.
tests/environments/local/local_env_telemetry_test.py Updated timestamp format strings to include microseconds.
tests/environments/local/composite_local_env_test.py Updated timestamp format strings to include microseconds.
tests/conftest.py Added docker_hostname fixture—missing sys import causes a bug.
tests/config/schedulers/test_load_scheduler_config_examples.py Replaced concrete SqlStorage type with the Storage interface.
tests/init.py Duplicate imports and two definitions of wait_docker_service_socket.
storage/sql/storage.py Added _reset_schema test helper.
storage/sql/schema.py Refactored string-length constants and added DATETIME(fsp=6).
storage/sql/experiment.py Removed forced truncation of microseconds in _new_trial.
storage/sql/alembic/versions/..._support_fractional_seconds_with_mysql.py Auto-generated Alembic revision for fractional-second support.
storage/sql/alembic/env.py Added custom_compare_types comparator and updated env configuration.
storage/sql/alembic/README.md Expanded and clarified migration instructions.
storage/sql/alembic.ini Updated hooks and uncommented URL options.
config/storage/postgresql.jsonc Corrected database name.
config/storage/mysql.jsonc Corrected database name.
Comments suppressed due to low confidence (4)

mlos_bench/mlos_bench/tests/conftest.py:38

  • The docker_hostname fixture references sys.platform but sys is not imported at the top of this file. Add import sys.
if sys.platform != "win32" and resolve_host_name(HOST_DOCKER_NAME):

mlos_bench/mlos_bench/tests/init.py:19

  • [nitpick] Duplicate import of Services as DockerServices. Remove the redundant import to clean up the module namespace.
from pytest_docker.plugin import Services as DockerServices

mlos_bench/mlos_bench/tests/init.py:109

  • This helper is defined twice in this file (also around line 151). Consolidate to a single definition to avoid confusion.
def wait_docker_service_socket(docker_services: DockerServices, hostname: str, port: int) -> None:

mlos_bench/mlos_bench/storage/sql/alembic/env.py:15

  • [nitpick] Importing two different Column classes under similar names may be confusing. Consider renaming the alias (e.g., InspectedColumn) for clarity.
from sqlalchemy.schema import Column as SchemaColumn

@bpkroth bpkroth added the ready for review Ready for review label Jun 11, 2025
@bpkroth bpkroth enabled auto-merge (squash) June 13, 2025 04:34
@bpkroth bpkroth merged commit 1bed08e into microsoft:main Jun 13, 2025
16 checks passed
@bpkroth bpkroth deleted the feature/mysql-schema-change-to-support-fractional-time branch June 13, 2025 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants