LuciferYang commented on PR #12414: URL: https://github.com/apache/gravitino/pull/12414#issuecomment-5511728975
@diqiu50 Done in 270a2b0cf, with main merged in. Three of the four inline comments were right and are fixed. `registerGravitinoCatalogs` now maps the provider to a kind once, and every decision below it is made from the kind, so `registerCatalog` no longer carries a provider that could disagree with it. `isPresent` no longer swallows: `ClassNotFoundException` and `LinkageError` are caught separately and each logs its reason with the throwable. Both log at debug, since both are the normal state of a 3.5 deployment that does not add the Paimon runtime, and the warning a user needs already fires later, when a config asks for Paimon and the build has none. If you would rather have `warn` on the `LinkageError` branch, I will change it. The Spark 4 authorization extension's javadoc pointed at `src/main/spark35`, which stopped being where the 3.5 copy lives once the plugin moved into the version modules. `SparkTableChangeConverter` I have left alone, and would like your call on it. `UpdateColumnDefaultValue.newDefaultValue()` returns a `String` on 3.5 and 4.0 alike, so a string literal is all the connector has at that point, and the code is not new here: it is `SparkTableChangeConverter34` from main, folded into the shared converter unchanged by the first commit. There is a real cost downstream. A string literal is not a `NumericType`, so the JDBC layer quotes it and an int column's default goes out as `DEFAULT '42'`; MySQL and PostgreSQL accept that, but the type is lost in Gravitino's metadata. Fixing it needs the column type at a seam that only receives field names and the default text, so it would have to load the table, which changes behaviour on 3.5 as much as on 4.0. I would rather do that as a separate issue than widen this PR. On coverage: the Iceberg REST backend was a real gap and now has a 4.0 subclass, along with the two authorization ITs. All three are picked up without a workflow change, since the Spark IT job selects by package. The other two you named are already covered on 4.0, which the diff does not make obvious. `TestRequiredPrivilegesSparkResolution` sits in the shared test tree, so it compiles into the 4.0 module and starts a real Spark 4 session with the extension configured; deleting `injectPostHocResolutionRule` from the 4.0 copy turns that module red while 3.5 stays green. Procedure loading runs through `testIcebergCallOperations` in the shared Iceberg suite, which `SparkIcebergCatalogHiveBackendIT40` inherits, and building the procedure against the internal Spark catalog instead of `this` fails it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
