-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PoC for Projection implementation for API Tables #1315
Changes from all commits
7a76d3e
e766521
1c209f3
f102417
b12038d
aff150d
7ffd494
327f030
85f9957
3353ce3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,17 +21,12 @@ | |
import java.util.Optional; | ||
import java.util.function.Supplier; | ||
import java.util.stream.StreamSupport; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
/** | ||
* TODO: this is still a POC class, showing how we can build a filter still to do is order and | ||
* projections | ||
*/ | ||
public class FindTableOperation extends TableReadOperation { | ||
|
||
private static final Logger LOGGER = LoggerFactory.getLogger(FindTableOperation.class); | ||
|
||
private final OperationProjection projection; | ||
private final FindTableParams params; | ||
|
||
|
@@ -72,7 +67,7 @@ public Uni<Supplier<CommandResult>> execute( | |
|
||
select = select.limit(params.limit()); | ||
|
||
// Building a statement using the positional values added by the TableFilter | ||
// Building a statment using the positional values added by the TableFilter | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like a typo There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Needs to be fixed in a follow-up. |
||
var statement = select.build(positionalValues.toArray()); | ||
|
||
// TODO: pageSize for FindTableOperation | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
package io.stargate.sgv2.jsonapi.service.operation.tables; | ||
|
||
import com.datastax.oss.driver.api.core.cql.Row; | ||
import com.datastax.oss.driver.api.core.metadata.schema.ColumnMetadata; | ||
import com.datastax.oss.driver.api.querybuilder.select.Select; | ||
import com.datastax.oss.driver.api.querybuilder.select.SelectFrom; | ||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
import com.fasterxml.jackson.databind.node.ObjectNode; | ||
import io.stargate.sgv2.jsonapi.exception.ErrorCode; | ||
import io.stargate.sgv2.jsonapi.service.cqldriver.executor.TableSchemaObject; | ||
import io.stargate.sgv2.jsonapi.service.operation.DocumentSource; | ||
import io.stargate.sgv2.jsonapi.service.operation.filters.table.codecs.JSONCodec; | ||
import io.stargate.sgv2.jsonapi.service.operation.filters.table.codecs.JSONCodecRegistry; | ||
import io.stargate.sgv2.jsonapi.service.operation.filters.table.codecs.MissingJSONCodecException; | ||
import io.stargate.sgv2.jsonapi.service.operation.filters.table.codecs.ToJSONCodecException; | ||
import io.stargate.sgv2.jsonapi.service.projection.TableProjectionDefinition; | ||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
|
||
/** | ||
* Projection used for Table Rows (as opposed to Collection Documents), built from command API | ||
* projection definitions (expressed in JSON). | ||
*/ | ||
public record TableRowProjection( | ||
ObjectMapper objectMapper, TableSchemaObject table, List<ColumnMetadata> columns) | ||
implements OperationProjection { | ||
/** | ||
* Factory method for construction projection instance, given a projection definition and table | ||
* schema. | ||
*/ | ||
public static TableRowProjection fromDefinition( | ||
ObjectMapper objectMapper, | ||
TableProjectionDefinition projectionDefinition, | ||
TableSchemaObject table) { | ||
Map<String, ColumnMetadata> columnsByName = new HashMap<>(); | ||
// TODO: This can also be cached as part of TableSchemaObject than resolving it for every query. | ||
table | ||
tatu-at-datastax marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.tableMetadata | ||
.getColumns() | ||
.forEach((id, column) -> columnsByName.put(id.asInternal(), column)); | ||
|
||
List<ColumnMetadata> columns = projectionDefinition.extractSelectedColumns(columnsByName); | ||
|
||
// TODO: A table can't be with empty columns. Think a redundant check. | ||
if (columns.isEmpty()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A table can't be with empty columns. Think a redundant check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Left a TODO, just in case it breaks anything There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are not checking whether table has no columns but whether set of columns specified by projection definition matching actual columns is empty. |
||
throw ErrorCode.UNSUPPORTED_PROJECTION_DEFINITION.toApiException( | ||
"did not include any Table columns"); | ||
} | ||
|
||
return new TableRowProjection(objectMapper, table, columns); | ||
} | ||
|
||
@Override | ||
public Select forSelect(SelectFrom selectFrom) { | ||
return selectFrom.columnsIds(columns.stream().map(ColumnMetadata::getName).toList()); | ||
} | ||
|
||
@Override | ||
public DocumentSource toDocument(Row row) { | ||
ObjectNode result = objectMapper.createObjectNode(); | ||
for (int i = 0, len = columns.size(); i < len; ++i) { | ||
final ColumnMetadata column = columns.get(i); | ||
final String columnName = column.getName().asInternal(); | ||
JSONCodec codec; | ||
|
||
// TODO: maybe optimize common case of String, Boolean to avoid conversions, lookups | ||
try { | ||
codec = JSONCodecRegistry.codecToJSON(table.tableMetadata, column); | ||
} catch (MissingJSONCodecException e) { | ||
throw ErrorCode.UNSUPPORTED_PROJECTION_PARAM.toApiException( | ||
"Column '%s' has unsupported type '%s'", columnName, column.getType().toString()); | ||
} | ||
try { | ||
result.put(columnName, codec.toJSON(objectMapper, row.getObject(i))); | ||
} catch (ToJSONCodecException e) { | ||
throw ErrorCode.UNSUPPORTED_PROJECTION_PARAM.toApiException( | ||
e, | ||
"Column '%s' has invalid value of type '%s': failed to convert to JSON: %s", | ||
columnName, | ||
column.getType().toString(), | ||
e.getMessage()); | ||
} | ||
} | ||
return () -> result; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,152 @@ | ||
package io.stargate.sgv2.jsonapi.service.projection; | ||
|
||
import com.fasterxml.jackson.databind.JsonNode; | ||
import io.stargate.sgv2.jsonapi.exception.ErrorCode; | ||
import java.math.BigDecimal; | ||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.Map; | ||
|
||
/** | ||
* Class similar to {@link DocumentProjector} but used for Table API, for non-nested | ||
* inclusion/exclusion projections. | ||
*/ | ||
public class TableProjectionDefinition { | ||
// Include-all is "exclude nothing" | ||
private static final TableProjectionDefinition INCLUDE_ALL_PROJECTOR = | ||
new TableProjectionDefinition(false, Collections.emptyList()); | ||
|
||
// Exclude-all is "include nothing" | ||
private static final TableProjectionDefinition EXCLUDE_ALL_PROJECTOR = | ||
new TableProjectionDefinition(true, Collections.emptyList()); | ||
|
||
private final boolean inclusion; | ||
|
||
private final List<String> columnNames; | ||
|
||
private TableProjectionDefinition(boolean inclusion, List<String> columnNames) { | ||
this.inclusion = inclusion; | ||
this.columnNames = columnNames; | ||
} | ||
|
||
public static TableProjectionDefinition createFromDefinition(JsonNode projectionDefinition) { | ||
// First special case: "simple" default projection; "include all" | ||
if (projectionDefinition == null || projectionDefinition.isEmpty()) { | ||
return INCLUDE_ALL_PROJECTOR; | ||
} | ||
if (!projectionDefinition.isObject()) { | ||
throw ErrorCode.UNSUPPORTED_PROJECTION_DEFINITION.toApiException( | ||
"must be OBJECT, was %s", projectionDefinition.getNodeType()); | ||
} | ||
// Special cases: "star-include/exclude" | ||
if (projectionDefinition.size() == 1) { | ||
Map.Entry<String, JsonNode> entry = projectionDefinition.fields().next(); | ||
if ("*".equals(entry.getKey())) { | ||
boolean includeAll = extractIncludeOrExclude(entry.getKey(), entry.getValue()); | ||
if (includeAll) { | ||
return INCLUDE_ALL_PROJECTOR; | ||
} | ||
return EXCLUDE_ALL_PROJECTOR; | ||
} | ||
} | ||
return createFromNonEmpty(projectionDefinition); | ||
} | ||
|
||
private static TableProjectionDefinition createFromNonEmpty(JsonNode projectionDefinition) { | ||
List<String> columnNames = new ArrayList<>(); | ||
boolean inclusionProjection = false; | ||
|
||
var it = projectionDefinition.fields(); | ||
while (it.hasNext()) { | ||
var entry = it.next(); | ||
String path = entry.getKey(); | ||
|
||
if (path.isEmpty()) { | ||
throw ErrorCode.UNSUPPORTED_PROJECTION_PARAM.toApiException( | ||
"empty paths (and path segments) not allowed"); | ||
} | ||
|
||
// Special rule for "*": only allowed as single root-level entry; | ||
if ("*".equals(path)) { | ||
throw ErrorCode.UNSUPPORTED_PROJECTION_PARAM.toApiException( | ||
"wildcard ('*') only allowed as the only root-level path"); | ||
} | ||
JsonNode value = entry.getValue(); | ||
boolean addInclusion = extractIncludeOrExclude(path, value); | ||
|
||
// If the first entry, we know inclusion/exclusion; if other, need to ensure | ||
// there's no mixing of inclusion/exclusion | ||
if (columnNames.isEmpty()) { | ||
inclusionProjection = addInclusion; | ||
} else if (inclusionProjection != addInclusion) { | ||
if (addInclusion) { | ||
throw ErrorCode.UNSUPPORTED_PROJECTION_PARAM.toApiException( | ||
"cannot include '%s' on exclusion projection", path); | ||
} else { | ||
throw ErrorCode.UNSUPPORTED_PROJECTION_PARAM.toApiException( | ||
"cannot exclude '%s' on inclusion projection", path); | ||
} | ||
} | ||
columnNames.add(path); | ||
} | ||
return new TableProjectionDefinition(inclusionProjection, columnNames); | ||
} | ||
|
||
/** | ||
* Method that selects columns from a map of column definitions, based on this projection | ||
* definition. | ||
* | ||
* @param columnDefs Column definitions by matching name to proper identifier | ||
* @return Filtered List of matching columns | ||
* @param <T> Actual column identifier type | ||
*/ | ||
public <T> List<T> extractSelectedColumns(Map<String, T> columnDefs) { | ||
// "missing" root layer used as short-cut for include-all/exclude-all | ||
if (columnNames.isEmpty()) { | ||
if (inclusion) { // exclude-all | ||
return Collections.emptyList(); | ||
} | ||
// include-all | ||
return columnDefs.values().stream().toList(); | ||
} | ||
|
||
// Otherwise need to actually determine | ||
List<T> included = new ArrayList<>(); | ||
|
||
if (inclusion) { | ||
for (String columnName : columnNames) { | ||
T columnDef = columnDefs.get(columnName); | ||
if (columnDef != null) { | ||
included.add(columnDef); | ||
} | ||
} | ||
} else { | ||
for (Map.Entry<String, T> entry : columnDefs.entrySet()) { | ||
if (!columnNames.contains(entry.getKey())) { | ||
included.add(entry.getValue()); | ||
} | ||
} | ||
} | ||
|
||
return included; | ||
} | ||
|
||
private static boolean extractIncludeOrExclude(String path, JsonNode value) { | ||
if (value.isNumber()) { | ||
// "0" means exclude (like false); any other number include | ||
return !BigDecimal.ZERO.equals(value.decimalValue()); | ||
} | ||
if (value.isBoolean()) { | ||
return value.booleanValue(); | ||
} | ||
if (value.isObject()) { | ||
throw ErrorCode.UNSUPPORTED_PROJECTION_PARAM.toApiException( | ||
"path ('%s') value cannot be OBJECT: nesting not supported for Tables", path); | ||
} | ||
|
||
// Unknown JSON node type; error | ||
throw ErrorCode.UNSUPPORTED_PROJECTION_PARAM.toApiException( | ||
"path ('%s') value must be NUMBER or BOOLEAN, was %s", path, value.getNodeType()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these be handled in the resolvers? These throws unhandled errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on how we view this: if there is no way to trigger it from outside (which I think is the case), these are assertions for internal state and should be ok this way.
But if there was a way to get null here from request it should throw
JsonApiException
.