Yohahaha commented on PR #11845: URL: https://github.com/apache/gravitino/pull/11845#issuecomment-5056912452
> > > @Yohahaha There is only one minor point that needs to be changed. Could you add unit test about this feature? All the other issues have been solved. > > > > > > Thanks for the review! @lasdf1234 I think current testPaimonCompactProcedure already covers this new functionality. > > In the connector module of Flink, there is a unit test named TestGravitinoPaimonCatalog. Saprk would also be well-advised to have such an UT in place. Even if there are additional unique logics added later, this approach can still be adopted. Thanks for the suggestion @lasdf1234 The Flink TestGravitinoPaimonCatalog covers complex logic like enrichCatalogTable hooks, cache invalidation, and metadata sync — which justifies the extensive mock-based unit tests. The change in this PR is much simpler — loadProcedure() is essentially a 2-branch delegation method (check namespace → build procedure → pass this as catalog). The existing testPaimonCompactProcedure integration test already covers the full end-to-end path: namespace validation, procedure builder creation, correct catalog passing, and actual procedure execution against a real Paimon catalog. Adding a TestGravitinoPaimonCatalog for Spark would make sense as a follow-up if more complex logic is added to the class in the future, but I think it's out of scope for this PR given the size of the change. -- 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]
