Add no-index support for Shredder (part 1) #786
Conversation
…t change anything
| this( | ||
| allowed, | ||
| denied, | ||
| Suppliers.memoize(() -> DocumentProjector.createForIndexing(allowed, denied))); |
There was a problem hiding this comment.
This is done so that DocumentProjector is only created first time it is needed (if at all) and reused if used in future (as CollectionSettings are effectively cached)
tatu-at-datastax
changed the title
| } | ||
|
|
||
| public DocumentProjector indexingProjector() { | ||
| return indexedProject.get(); |
There was a problem hiding this comment.
This does build the Projector for every call. Is the idea to get this once use it in code passing everywhere?
There was a problem hiding this comment.
No, it uses Guava's memoize() to avoid just that.
There was a problem hiding this comment.
I'll try to come up with a test to verify that we only get one instance; maybe CreateCollectionCommandResolverTest?
There was a problem hiding this comment.
Ok, easier to verify with stand-alone unit test: see CollectionSettingsTest which verifies that instance is created dynamically, and then same instance returned for further calls to get instance.
| // Note that (5) is effectively same as (3) and included for sake of uniformity | ||
| if (allowed != null && !allowed.isEmpty()) { | ||
| // (special) Case 5: | ||
| if (allowed.size() == 1 && allowed.contains("*")) { |
There was a problem hiding this comment.
allowed can't have *, it's only supported for denied.
There was a problem hiding this comment.
That is something I wanted to discuss (should have brought up) -- for consistency it would make sense to allow this. But for minimal approach not. Will ask on Stargate channel.
My main concern is if users would pass this, assuming it works, and it meaning something totally different.
| """; | ||
|
|
||
| // Try with overlapping paths | ||
| assertDenyProjection(Arrays.asList("a", "a.b"), INPUT_DOC, EXP_OUTPUT); |
There was a problem hiding this comment.
What will be the output if denied list is only "a.b"? Will we index field "a.b.z"?
There was a problem hiding this comment.
We take most generic path, first, so this is same as only using "a".
And so a, a.b, a.b.c would all be excluded (not indexed).
| // First with empty Sets: | ||
| assertAllowProjection(Arrays.asList(), DOC, DOC); | ||
| // And then "*" notation | ||
| assertAllowProjection(Arrays.asList("*"), DOC, DOC); |
There was a problem hiding this comment.
Related to earlier comment: so, correct, depends on our definition.
src/test/java/io/stargate/sgv2/jsonapi/service/projection/DocumentProjectorForIndexingTest.java
Show resolved
Hide resolved
What this PR does:
Adds index allow/deny handling to
Shredderbased onIndexingConfigadded via CreateCollectionWhich issue(s) this PR fixes:
Fixes #767
Checklist