Repository navigation
[#34613] Fix SqlTransformSchemaTransformProvider config schema for cross-language use - #40926
Brijesh-Thakkar wants to merge 1 commit into
Conversation
…he#34613) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
📋 Review Summary
This PR resolves #34613 by replacing Java-specific logical types (EnumerationType and OneOfType) in SqlTransformSchemaTransformProvider.configurationSchema() with portable primitive types (STRING and ARRAY<STRING>) and removing the unused parameters field so the schema can be decoded by non-Java SDKs such as Python. It also adds explicit validation for dialect and tableproviders in from(Row) along with unit tests in both Java and Python.
🔍 General Feedback
- The migration from
EnumerationTypetoSTRING/ARRAY<STRING>with validation infrom(Row)preserves type safety and improves error reporting over the previous implementation (which silently ignored unknown table providers). - The new Java unit tests (
SqlTransformSchemaTransformProviderTest) and Python schema decoding tests (SqlTransformSchemaTransformConfigTest) provide solid coverage for schema portability, configuration validation, and transform expansion.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
Assigning reviewers: R: @jrmccluskey for label python. This pull request likely touches a core component ("core" label). Please review with scrutiny. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Summary
SqlTransformSchemaTransformProvider(schematransform:org.apache.beam:sql_transform:v1) could not be discovered from Python: its configuration schema used Java-only logical types that Python cannot decode, sotyping_from_runner_api/named_tuple_from_schemafailed withNo logical type registered for URN .... The offending fields were:dialect:EnumerationType(beam:logical_type:javasdk_enum:v1)tableproviders:ARRAY<EnumerationType>(javasdk_enum:v1)parameters:OneOfType(beam:logical_type:javasdk_oneof:v1)This PR makes the schema use only portable types:
dialectis now a nullableSTRING.from()rejects values that are not a registered query planner (currentlycalcite).tableprovidersis now a nullableARRAY<STRING>.from()rejects names that are not the table type of a registeredTableProvider. The old enum enforced this implicitly, and the old code silently ignored unknown names.parametersis removed. It was never read byfrom()orexpand()(there was only a TODO), so no functionality is lost. It can return later using plain types once it is actually implemented.Why not a Python-side workaround
The earlier attempt in #34605 changed only the Python wrapper and was closed unmerged; its author noted it could not work because of this issue. The schema itself has to be fixed.
Compatibility
The config schema changes shape for Java callers: anyone who built a config
RowwithEnumerationType.Values, or setparameters, must now pass plain strings and dropparameters. The discussion on #34613 found no users of this provider. Validation is still enforced, now infrom()with a clearIllegalArgumentExceptionthat lists the allowed values.YAML's
Sqltransform (yaml/standard_providers.yaml) passes onlyquery, so it should be unaffected. I readyaml_mapping.py,yaml_join.pyandyaml_combine.pybut did not run the YAML tests.Testing
Java (
:sdks:java:extensions:sql:expansion-service:test --tests '*SqlTransformSchemaTransformProvider*', and:check): 6 tests pass. They cover:parameters;dialect=calcite;ddl(aCREATE EXTERNAL TABLE ... TYPE 'text'over a temp file, then a query reading from it). Note that DDL table types resolve only withautoload=true; withautoload=falsethetableprovidersentries are registered as named schemas. That is existing behavior and is unchanged here.With only
SqlTransformSchemaTransformProvider.javareverted to master, the 5 original tests fail:testConfigurationSchemaUsesOnlyPortableTypesfails on its assertion (parametersis present), and the other 4 fail withClassCastException(String cannot be cast to EnumerationType$Value) while building the configRow, because the old enum fields reject plain strings. That is the same non-portability as the bug. With the fix they pass. The DDL test was added afterwards and was not run against master.Manual cross-language check (not automated in CI): I built the expansion-service shadow jar locally and drove it from Python.
dialect='calcite'works and returnsoutputanderrors.dialect='bogus'andtableproviders=['nope']raise clearIllegalArgumentExceptions through Python.Python (
sql_test.py):SqlTransformSchemaTransformConfigTestis a regression guard, not an end-to-end test. It hand-builds the proto that the Java schema emits and checks thatnamed_tuple_from_schemadecodes it, and that the old enum logical type still does not decode. It does not start an expansion service, and the Java side is pinned by the Java test, so the two copies must be kept in sync by hand.pytest apache_beam/transforms/sql_test.py: 2 passed (the other 11 tests in the file are existing cross-language tests that skip without a runner). yapf and ruff are clean;spotlessApply/spotlessCheckare clean.Not addressed
ExternalTransformProviderstill skips this transform by default: the identifierschematransform:org.apache.beam:sql_transform:v1has nobeam:prefix, so it does not match the default URN pattern (^beam:schematransform:org.apache.beam:([\w-]+):(\w+)$). It works with a customurn_pattern. I left it out becauseyaml/standard_providers.yamldepends on the identifier. A follow-up issue will be filed.Generated with Claude Code
Fixes #34613