feat: add missing extension YAMLs and test for completeness#723
feat: add missing extension YAMLs and test for completeness#723benbellick merged 5 commits intomainfrom
Conversation
benbellick
force-pushed
the
add-missing-extensions-test
branch
19 times, most recently
from
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. |
benbellick
force-pushed
the
add-missing-extensions-test
branch
from
022b315 to
0f694f2
Compare
benbellick
changed the title
|
This was rebased off of #726, so I will wait for that one to be merged before merging this one. |
benbellick
force-pushed
the
add-missing-extensions-test
branch
from
0f694f2 to
f6b2f93
Compare
benbellick
force-pushed
the
add-missing-extensions-test
branch
5 times, most recently
from
6d9545e to
22e6958
Compare
benbellick
force-pushed
the
add-missing-extensions-test
branch
from
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
- Switch test to classpath-based discovery instead of filesystem path - Load geometry and aggregate_decimal_output extensions - Remove duplicate FUNCTIONS_LIST constant and stale TODO - Verify unsupported files are NOT in catalog (prevents stale skip list) - Reduce skip list to only type_variations and unknown Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
benbellick
force-pushed
the
add-missing-extensions-test
branch
from
c288cc1 to
2e8ab69
Compare
Move extension YAML files from the classpath root into a substrait/extensions/ subdirectory for better organization. Revert geometry and aggregate_decimal_output additions that break isthmus tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the filesystem hack (locating a known resource and walking its parent directory) with ClassGraph's classpath scanning API. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
2 participants
Closes #722
Adds a test that dynamically discovers all extension YAMLs under
substrait/extensions/on the classpath (via ClassGraph) and verifies they are loaded byDefaultExtensionCatalog. Files not yet loadable are listed inUNSUPPORTED_FILESwith comments explaining why. The test also asserts these are NOT loaded, so the skip list can't silently go stale.Also relocates extension YAMLs from the classpath root into
substrait/extensions/, adds theFUNCTIONS_AGGREGATE_DECIMAL_OUTPUTconstant, and loadsfunctions_set.