feat: add missing extension YAMLs and test for completeness#723
feat: add missing extension YAMLs and test for completeness#723benbellick wants to merge 1 commit intomainfrom
Conversation
8bc6344 to
022b315
Compare
|
ACTION NEEDED Substrait follows the Conventional Commits The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
022b315 to
0f694f2
Compare
|
This was rebased off of #726, so I will wait for that one to be merged before merging this one. |
0f694f2 to
f6b2f93
Compare
6d9545e to
22e6958
Compare
22e6958 to
c288cc1
Compare
|
As noted when |
| File extensionsDir = new File("../substrait/extensions"); | ||
| assertTrue(extensionsDir.isDirectory(), "substrait/extensions directory not found"); |
There was a problem hiding this comment.
I'm wondering whether this should check the extension YAML files on the classpath instead of relying on the current working directory (especially given that @vbarua is planning to remove the submodule and move packaging the extension YAMLs into a separate JAR into substrait-packaging). The YAMLs should be already on the classpath. We're already loading classgraph for the isthmus-cli which can be useful to scan the classpath for any *.yaml files.
There was a problem hiding this comment.
Sounds good to me, I'll make that change shortly. Thanks!
There was a problem hiding this comment.
btw. it might be better to relocate the YAML files on the classpath into a subfolder like substrait/extensions to be easier recognizable as default YAML files. right now they are being added into the root of the classpath
Closes #722
Adds
functions_settoDefaultExtensionCatalogand a test that dynamically discovers all extension YAMLs insubstrait/extensions/and verifies they are loaded, so future omissions are caught automatically.Files not yet loaded are listed in the test's
UNSUPPORTED_FILESset with inline comments explaining why.