laserninja commented on PR #11216: URL: https://github.com/apache/gravitino/pull/11216#issuecomment-5548917890
Hi @ibrahimErbilen, thanks for working on this - a `rewrite_manifests` job is a genuinely useful addition and the shape of the change looks right. The PR has been open since May with the review comments unaddressed. Are you still able to pick it back up? Most of them are quick: - `BuiltInJobTemplateProvider`: the closing `)` of `ImmutableList.of(...)` is one line too early, so `new IcebergRewriteManifestsJob()` sits outside the list and the module doesn't compile. - The new import is below the `org.slf4j` ones, breaking import ordering. - `testJobTemplateHasMainClass` asserts on `template.className()`. One is worth a bit more thought: the job reaches into `IcebergRewriteDataFilesJob` for `parseCustomSparkConfigs`, `escapeSqlIdentifier`, and `escapeSqlString`. Pulling those into a shared helper would avoid coupling two sibling job implementations together. If you'd rather hand it off, that's completely fine - just say so here and I'll carry it forward on top of your commits so your authorship is preserved. If there's no reply in about a week I'll assume that works and open a follow-up PR keeping you as the author of the original commits. -- 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]
