Repository navigation
CADC-15541: Youcat: update table-ops to match ops WD - #267
aratikakadiya wants to merge 6 commits into
Conversation
aratikakadiya
commented
Aug 6, 2026
aratikakadiya
commented
- Added support for multicolumn index -- updated params structure according to the openapi doc.
- Added int test to test unique and multicolumn index with latest param structure.
- Added support for multicolumn index - Added int test to test unique and multicolumn index with latest param structure.
brianmajor
left a comment
brianmajor
left a comment
There was a problem hiding this comment.
I took a quick look and generally the code looks quite well constructed. I like how you plan to transition to the multi-column index by just throwing the UnsupportedOperationException for now, but having the framework in place. I will leave this to @pdowler to complete the merge when ready.
Updated int test to validate index creation success for long-lat index type and failure for x-y index type.
|
|
||
| doCreateIndex(schemaOwner, tableName, List.of("c1", "c2"), null, "long-lat", ExecutionPhase.COMPLETED, null); | ||
| doCreateIndex(schemaOwner, tableName, List.of("c1", "c2"), null, "x-y", ExecutionPhase.ERROR, | ||
| "unexpected failure: failed to update table int_test_schema.testCreateMultiColIndex reason: x-y index type is not yet supported"); |
There was a problem hiding this comment.
if the response to this test starts with "unexpected failure" then we should fix that to actually output something like "unsupported: x-y index type".
normally, UnsupportedOperationException is used for things we have not implemented. iirc, both IllegalArgumentException and UOE map to 400 so on the client side of http our code maps them both to IllegalArgumentException (aka bad request)... so will take some care to make the reported message be correct.
aside: it was probably a bad idea to have two different thrown exceptions map to the same http code (400) but there is still a lot of code that does it.
| sb.append(" USING ").append(using); | ||
| sb.append(" INDEX ").append(indexName); | ||
| sb.append(" ON ").append(first.getTableName()); | ||
| if (indexType != null && (indexType.equalsIgnoreCase("long-lat") || indexType.equalsIgnoreCase("x-y"))) { |
There was a problem hiding this comment.
All the index-type values must be passed to ddType.getIndexUsingQualifier(...) to the PG-specific code can generate the USING (or not). So the correct thing is to change the method signature of that method from (ColumnDesc, boolean) to (List, List) and fix the PG implementation.
| if (iop != null) { | ||
| sb.append(" ").append(iop); | ||
|
|
||
| if (indexType != null && indexType.equalsIgnoreCase("long-lat")) { |
There was a problem hiding this comment.
like above, but not that we have multi-column indixes we need to replace line 376-379 with a call to ddType to generate the expression (because it is PG-specific).
That means modifying the DatabaseDataType interface, maybe to combine these variable things?? We can rely on
CREATE [UNIQUE] INDEX <name> on <table_name> ...
but it seems like everything else could be backend specific. Maybe
String getIndexExpression(List<ColumnDesc> columns, List<String> indexTypes)
which has to return the ( ... ) and anything else that is needed. The default implementation (for no indexTypes) in BasicDataTypeMapper could just do the plain list of columns). I think this single method could make both getIndexUsingQualifier and getIndexColumnOperator obsolete.
| return sb.toString(); | ||
| } | ||
|
|
||
| private static String buildSpointExpression(List<ColumnDesc> columns, ColumnDesc column) { |
There was a problem hiding this comment.
this belongs in the cadc-tap-server-pg` library
3 participants
