Conversation
|
@a-random-steve FYI this is not small but I think the way it's structured makes sense with the rest 🙏 |
|
Note. A detail that could be added is constraining the column type to the index type (when applicable), for typed column argument:
However I think this can be added later (trading an API exception for a safer typing error, I wouldn't say it's a problem to do it post-GA) and also I'm not 100% sure this fits with the tenet that clients should not "take initiative". |
|
Hold on: I have to investigate a glitchy case @skedwards88 reported today. Stay tuned... |
| return _column; | ||
| } | ||
| internal set => _column = value is JsonElement je | ||
| ? DeserializationUtils.UnwrapJsonElement(je) |
There was a problem hiding this comment.
@sl-at-ibm This solution for handling the challenges of the variable value makes sense. Currently the client doesn't handle any of these sorts of serdes issues directly in the class, so for consistency I might slightly prefer another JsonConverter in SerDes instead. Thoughts?
There was a problem hiding this comment.
I agree that this belongs more to the serdes layer.
However, I have lifted this part straight from TableIndexDefinition.cs as-is, where it was already exactly like that.
Here is what I'd do: we can merge as is and open a "code tidy" issue (post-GA) on this item. This way no further blocking happens because of this already pretty big PR. (I understand the improvement results in zero externally-visible effects).
There was a problem hiding this comment.
Makes sense. Agreed. And WHO put that in TableIndexDefinition? 😮 lol
…ns (regular, text, vector)
…ommand for untyped index creation
7d745bc to
71b3d42
Compare
|
@a-random-steve I changed my mind and will go with the proper SerDes refactoring. Stay tuned. The reason is that I found another couple of things to change/add which make sense to do within this PR:
@toptobes so please hold off and do not merge this one still :| |
Fixes #36 .
Fixes #37.
Fixes #38.
Fixes #39.
(#41 seems fixed already)
This PR changes the table index creation API significantly as outlined on #36. Some guidance to reviewers is in order:
ListIndexesAsync(...)response post-creation is left to a future task.CreateIndexCommandOptions). This PR has not changed the behaviour to "manually" extract IfNotExists from this object and inject it into the payload, which might need to be revisited (but it would be a breaking change). SeeCreateGenericIndexAsync.CreateIndexneeds a way to specify$keys/$values#41 claims, indexes on maps are fully functional. FourCreateIndexTests_MapIndex_*tests pass and their payload has been verified when completing this PR, so issue 41 should be good to close IMO.TableIndexDefinitionflattens/deepens, in-and-out, the Ascii/CaseSensitive/Normalize options. While this offers a leaner API to the user, it distorts the underlying payload (which has them underoptions). It might be worth revisiting this logic as it would become unwieldy should the Data API ever enrich the options tree (admittedly unlikely, but...). Just drawing attention to this item as it is potentially breaking change.TableIndexBaseDefinitionclass, whichTableIndexDefinitioninherits from just like the vector and text counterpart. This was critical to prevent the possibility to create frankenstein options (and payloads) with e.g., both ascii/normalize and vector settings, or similar. Note that the naming pattern, withTableIndexDefinitiona subclass, is chosen for alignment to the other clients and, more important, to the command names the Data API exposes.options: {}is passed if no such settings are provided.CreateIndexshould be split into three separate methods #37 (splitting of the methods) is of course a greatly breaking change (@skedwards88 cc) with big impact on the docs as well. (and quite a proliferation of the methods in Table.cs).