[Coral-Schema] Add CoralCatalog base-table and table-scan paths for Avro schema generation#604
Conversation
yyy1000
force-pushed
the
iceberg-first-avro/pr5-pr6-coral-catalog-paths
branch
from
ec53682 to
84af052
Compare
| * @param hiveMetastoreClient client used for view conversion and as a fallback; must not be null | ||
| * @return a new converter wired to both | ||
| */ | ||
| public static ViewToAvroSchemaConverter create(CoralCatalog coralCatalog, HiveMetastoreClient hiveMetastoreClient) { |
There was a problem hiding this comment.
not aligned with this user facing method. from a UX perspective, if a user is looking to generate a view's avro schema , they will either pass CoralCatalog or HiveMetastoreClient depending on whether they are on new or old variant.
we should -
(1) expose a public static - public static ViewToAvroSchemaConverter create(CoralCatalog coralCatalog) {..} method.
(2) mark public static ViewToAvroSchemaConverter create(HiveMetastoreClient hiveMetastoreClient) {..) deprecated.
There was a problem hiding this comment.
Done in dfff1b6 — create(CoralCatalog, HiveMetastoreClient) removed; new create(CoralCatalog) exposed; create(HiveMetastoreClient) marked @Deprecated. The new factory wires the inner HiveToRelConverter and RelToAvroSchemaConverter against the same CoralCatalog end-to-end, so the converter no longer mixes abstractions internally.
| } | ||
|
|
||
| Table baseTable = hiveMetastoreClient.getTable(dbName, tableName); | ||
| if (baseTable == null) { |
There was a problem hiding this comment.
this codepath makes we wonder that we are potentially allowing users to create a CoralRelNode using the old abstractions - HiveMetastoreClient and then create a view schema by merging coralRelNode generated using HiveMetastoreClient with CoralTable generated using CoralCatalog. that can give some weird bugs and else cases.
we need to decouple the two code paths a bit more.
the user's entry point is the class - ViewToAvroSchemaConverter (is that right?) . if yes, we need to wire it up such that there are 2 independent code paths (pseudo code example) -
(1)
viewToAvroSchemaConverter1 = new ViewToAvroSchemaConverter(HiveMetastoreClient);
viewToAvroSchemaConverter1.getAvroSchema(db,tbl)
-- this uses HiveToRelConverter(HiveMetastoreClient) & RelToAvroConverter(HiveMetastoreClient)
and
(2)
viewToAvroSchemaConverter2 = new ViewToAvroSchemaConverter(CoralCatalog);
viewToAvroSchemaConverter2.getAvroSchema(db,tbl)
-- this uses HiveToRelConverter(CoralCatalog) & RelToAvroConverter(CoralCatalog)
please check if we're enabling a code path which inter mixes the two interactions.
There was a problem hiding this comment.
Done in dfff1b6 — the two paths are now fully decoupled. SchemaRelShuttle branches on a single non-null field (CoralCatalog xor HiveMetastoreClient) with no fallback. A CoralCatalog-backed instance never resolves through HiveMetastoreClient and vice versa; if the configured catalog can't find the table it throws instead of silently degrading.
ViewToAvroSchemaConverter was also refactored along the same lines: create(CoralCatalog) constructs HiveToRelConverter(coralCatalog) + RelToAvroSchemaConverter(coralCatalog) end-to-end, and inferAvroSchema splits into two branches that share no state (inferAvroSchemaCoralCatalog and inferAvroSchemaHiveMetastore). A user can no longer build a CoralRelNode through HiveMetastoreClient and resolve base tables through CoralCatalog on the same converter instance.
There was a problem hiding this comment.
public facing API should only take either CoralCatalog or HiveMetastoreClient. please refactor the code.
There was a problem hiding this comment.
Done in dfff1b6 — (CoralCatalog, HiveMetastoreClient) constructor removed; new (CoralCatalog) constructor added; (HiveMetastoreClient) marked @Deprecated with a Javadoc pointer to the new constructor.
|
@yyy1000 thanks for the PR! keep the momentum going :) feel free to introduce new class variants , if needed, to enable the decoupling of new & old codepaths. And btw, the build is also failing. |
yyy1000
force-pushed
the
iceberg-first-avro/pr5-pr6-coral-catalog-paths
branch
from
84af052 to
dfff1b6
Compare
|
Pushed dfff1b6 with the decoupling refactor — replies posted on each of the three threads above. Also folded the |
|
|
||
| // Hive-backed CoralTable: delegate to the existing Hive-table API to preserve current behavior. | ||
| if (coralTable instanceof HiveTable) { | ||
| return getAvroSchemaForTable(((HiveTable) coralTable).getHiveTable(), strictMode); |
There was a problem hiding this comment.
getHiveTable() method has documentation:
/**
* INTERNAL API
* @deprecated This method is for internal use only and will be removed in a future release.
* Do not depend on this API.
*
* @return Hive metastore Table object
*/
please do not leverage this API. if the implementation requires a conversion from (HiveTable) CoralTable -> Hive's table object, implement the converter. Feel free to do that in coral-common so that other usages of that API can migrate too
There was a problem hiding this comment.
Agreed on dropping the deprecated getHiveTable(). Worth flagging two things before sketching a fix:
- PR [Coral-Schema] Add CoralCatalog base-table and table-scan paths for Avro schema generation #604 isn't the first caller.
HiveCalciteTableAdapter.java:122andToRelConverter.java:209already usegetHiveTable()today — both insidecoral-commonitself. Whatever we land here is the natural migration target for those too. - A real converter needs more of
HiveTable's state than the current public API exposes —serdeInfo.serializationLib(AvroSerDe escape), serdeparameters(partner-Avro extraction), and the cols/partition-keys split (today collapsed into oneStructTypebygetSchema()). Reconstructing a HiveTablefromname()+properties()+getSchema()alone would silently break the Hive arm.
Two ways forward:
- Expand
HiveTable's public API in a separate prep PR, then add a realHiveCoralTableConverterhere. - Add
HiveCoralTableConverterhere as a centralized facade that internally usesgetHiveTable()(with@SuppressWarnings), so the deprecated call lives in exactly one place and the existing in-tree usages migrate behind the same converter.
1 is the right end state, 2 is a pragmatic interim. Which would you prefer?
There was a problem hiding this comment.
can you think about more options from a design standpoint? how about we expand hiveTable.properties() method here to also include the serde properties in addition to the table properties - essentially that would truly capture the hive table's properties.
would that resolve the issue? what all aspects of 'hiveTable' are required across coral-common & coral-schema? we can limit scope to just coral-schema for now. what are the implications here? or should we implement a new getAvroSchemaForTable() without knowledge of the base hive table? is that possible?
please spend some time thinking about these options and more and share what you think should be adopted and why.
There was a problem hiding this comment.
Spent some time on this. My pick: targeted expansion of HiveTable's public API plus a shared private helper between the two getAvroSchemaForTable overloads.
Four new methods on HiveTable, scoped to what coral-schema's Hive path actually consumes:
public Map<String, String> serdeProperties() // sd.serdeInfo.parameters
public String serializationLib() // sd.serdeInfo.serializationLib
public List<FieldSchema> regularColumns() // sd.cols
public List<FieldSchema> partitionColumns() // partitionKeysThen in SchemaUtilities, extract the existing getAvroSchemaForTable(Table) body into a private helper that takes those fields plus tableProperties, fullyQualifiedName, strictMode. Both overloads (Table and CoralTable) become thin wrappers that route through it.
Result: bytecode-equivalent Hive behavior (same helper, same inputs), no getHiveTable() in coral-schema. Scope stays at "what coral-schema needs from HiveTable" per your framing — the other two in-tree usages (HiveCalciteTableAdapter, ToRelConverter) can migrate to these accessors in a follow-up without further API additions.
What do you think?
There was a problem hiding this comment.
i looked at Apache-Iceberg's Table API and serde properties are not retrievable using a top class API, it i shoved under properties. We should also look to not expand the user facing APIs extensively.
So maybe create a constant key for SERDE_SERIALIZATION and put the serde in it in the same properties key value map. But agree that this is going beyond the scope of coral-schema.
So for now lets log an issue for this follow up item, link this thread in the issue as well and link the issue in code as a TODO.
| String recordNamespace = lastDot >= 0 ? fullName.substring(0, lastDot) : ""; | ||
| String recordName = lastDot >= 0 ? fullName.substring(lastDot + 1) : fullName; |
There was a problem hiding this comment.
is this is how name & namespace is extracted for hive tables? shouldnt we prefer to extract is from the Avro schema partner, if that is present? lets keep parity with old behaviour
There was a problem hiding this comment.
Good catch — fixed in b8e6c1e. When partner Avro is present, MergeCoralSchemaWithAvro.mergeTopLevelStruct already takes name/namespace from the partner record (via copyRecord), so legacy parity is already there for that case. For the no-partner fallback, I now match convertHiveSchemaToAvro: recordName = the bare table name, recordNamespace = the fully qualified name (e.g. default.my_table). Updated the affected SchemaUtilitiesTests assertions too.
| Preconditions.checkNotNull(tableOrViewName); | ||
|
|
||
| if (coralCatalog != null) { | ||
| return inferAvroSchemaCoralCatalog(dbName, tableOrViewName, strictMode, forceLowercase); |
There was a problem hiding this comment.
in your regression testing, please make sure that you test for permutations of strictMode + forceLowercase as well.
There was a problem hiding this comment.
Tracking on internal testing side where the regression harness lives. Will extend CoralSparkBaselineAnalysisForTrino to emit one avro_schema column per (strictMode, forceLowercase) permutation so the diff query covers all four.
| * {@link CoralCatalog#getTable}; views use the CoralCatalog-aware | ||
| * {@link HiveToRelConverter} and {@link RelToAvroSchemaConverter}. | ||
| */ | ||
| private Schema inferAvroSchemaCoralCatalog(String dbName, String tableOrViewName, boolean strictMode, |
There was a problem hiding this comment.
nit: inferAvroSchemaUsingCoralCatalog
There was a problem hiding this comment.
Renamed in b8e6c1e. Also renamed the sibling inferAvroSchemaHiveMetastore → inferAvroSchemaUsingHiveMetastore for symmetry.
…ration
Wires the Coral side of the Iceberg-first Avro flow end-to-end as two
fully independent code paths -- one driven by CoralCatalog, one by the
legacy HiveMetastoreClient. Instances built from one abstraction never
mix with the other.
SchemaUtilities:
- New getAvroSchemaForTable(CoralTable, strictMode) overload.
- For HiveTable, delegates to the existing getAvroSchemaForTable(Table,
strictMode) so current Hive behavior is preserved unchanged.
- For other CoralTables (e.g. Iceberg-backed), reads partner Avro
from CoralTable.properties() via the property-map helper from
PR linkedin#589 and merges with the Coral schema using Iceberg-first
semantics via MergeCoralSchemaWithAvro.merge.
- Generates a pure Coral-derived schema when no partner Avro is
available; in strict mode this case throws SchemaNotFoundException.
ViewToAvroSchemaConverter:
- Two mutually exclusive public factories:
create(CoralCatalog) -- new path
create(HiveMetastoreClient) -- legacy, marked @deprecated
- Each factory wires its inner HiveToRelConverter and
RelToAvroSchemaConverter against the same abstraction, end to end.
- inferAvroSchema delegates to one of two private branches that share
no state with each other; views and tables are both surfaced
through the configured catalog/metastore. The CoralCatalog branch
uses CoralTable.tableType() to distinguish base tables from views.
- The previous (CoralCatalog, HiveMetastoreClient) factory is removed
so callers can no longer build a converter that mixes the two
abstractions internally.
RelToAvroSchemaConverter:
- Two mutually exclusive public constructors:
RelToAvroSchemaConverter(CoralCatalog) -- new path
RelToAvroSchemaConverter(HiveMetastoreClient) -- legacy, @deprecated
- SchemaRelShuttle holds both fields but exactly one is non-null per
instance; getTableScanSchema branches on that and never falls back
between the two abstractions. A CoralCatalog-backed shuttle that
cannot resolve a table now throws instead of silently degrading
through Hive metastore.
- The previous (CoralCatalog, HiveMetastoreClient) constructor is
removed.
coral-hive build:
- tasks.named('spotlessJava') now mustRunAfter both
generateGrammarSource and generateTestGrammarSource. Without the
test-grammar entry, Gradle 8 strict validation fails the
coral-hive build.
Tests: SchemaUtilitiesTests covers the new Iceberg path (with-partner,
no-partner non-strict, no-partner strict-throws, unqualified-name
fallback, plus a Coral-nullability-wins assertion proving the merge
engine ran). The HiveMetastoreClient-backed paths remain covered by
existing tests through their unchanged constructors and factories.
yyy1000
force-pushed
the
iceberg-first-avro/pr5-pr6-coral-catalog-paths
branch
from
dfff1b6 to
b8e6c1e
Compare
aastha25
changed the title
2 participants
Motivation
PR #600 added the
MergeCoralSchemaWithAvroengine but did not wire it into any call site. As a result, base tables and table scans inside views still resolve through the Hive-only path, even when the table is Iceberg-backed.This PR adds the Coral-side
CoralCatalogplumbing so base tables and table scans can resolve through the new merge engine. Hive- and Iceberg-backed base tables share a single dispatch path; Hive behavior is preserved unchanged for callers that don't opt in.Summary
SchemaUtilities.getAvroSchemaForTable(CoralTable, strictMode)— new overload.HiveTabledelegates to the existinggetAvroSchemaForTable(Table, strictMode). Other CoralTables (Iceberg-backed) parse partner Avro fromCoralTable.properties()viagetCasePreservedSchemaFromPropertyMaps(PR Refactor partner-Avro parsing into reusable property-map helper #589) and merge with the Coral schema usingMergeCoralSchemaWithAvro.merge. Strict mode throwsSchemaNotFoundExceptionwhen partner Avro is absent; non-strict mode emits a pure Coral-derived schema.ViewToAvroSchemaConverter.create(CoralCatalog, HiveMetastoreClient)— new factory + matching private constructor + optionalCoralCatalogfield. The existingcreate(HiveMetastoreClient)factory and constructor are unchanged.inferAvroSchema()dispatches base-table resolution through theCoralCatalogwhen one is provided; views fall through to the existing path because view conversion still requiresHiveToRelConverteron a Hive metastore.coralCatalogis also threaded into the innerRelToAvroSchemaConverter.RelToAvroSchemaConverter— new optionalCoralCatalogfield +(CoralCatalog, HiveMetastoreClient)constructor. The existing(HiveMetastoreClient)constructor delegates to it withnull.SchemaRelShuttle.getTableScanSchema()resolves throughcoralCatalog.getTable()first when set, falling back tohiveMetastoreClient.getTable()if the CoralCatalog does not surface the table.Behavior preservation
Every existing constructor/factory still exists. The new constructors delegate to the legacy ones with a
null coralCatalog, so the legacy code path is byte-for-byte identical. Callers that don't opt in see no behavior change.Test plan
SchemaUtilitiesTestscases for the Iceberg path:testGetAvroSchemaForCoralTableIcebergWithPartner— partner-provided record name preserved, fields populated.testGetAvroSchemaForCoralTableIcebergNoPartnerNonStrict— pure Coral-derived schema with name/namespace from the CoralTable name.testGetAvroSchemaForCoralTableIcebergNoPartnerStrictThrows—SchemaNotFoundExceptionraised.testGetAvroSchemaForCoralTableUnqualifiedNameYieldsEmptyNamespace— unqualified name handling.ViewToAvroSchemaConverter/RelToAvroSchemaConverterdispatches are covered transitively by existing tests on theHiveMetastoreClient-only constructors — the new constructors and factories are additive, andnull coralCatalogreproduces prior behavior.coral-schema:testsuite passes;spotlessJavaCheckclean.Follow-up
Complex multi-branch unions in partner Avro are not yet handled by
MergeCoralSchemaWithAvro(only[null, T]shapes are). A follow-up PR will extend the merge engine for union/fixed/UUID/Avro-only edge cases. Pure Iceberg base tables rarely carry multi-branch unions in their partner Avro, so this PR is safe to ship before that work; production regression testing on this path will inform the exact scope of the follow-up.