From 7c965e24776555d21833c7091e9f3fb20c1559fd Mon Sep 17 00:00:00 2001 From: awildturtok <1553491+awildturtok@users.noreply.github.com> Date: Thu, 10 Oct 2024 17:30:04 +0200 Subject: [PATCH] Fixes faulty calculation of width in TableExportQueryConverter (#3597) Fixes faulty calculation of width in TableExportQueryConverter by using unified method from TableExportQuery#calculateWidth --------- Co-authored-by: Jonas Arnhold --- .../apiv1/query/TableExportQuery.java | 64 ++++---- .../query/queryplan/TableExportQueryPlan.java | 3 +- .../query/TableExportQueryConverter.java | 150 ++++++++++-------- 3 files changed, 113 insertions(+), 104 deletions(-) diff --git a/backend/src/main/java/com/bakdata/conquery/apiv1/query/TableExportQuery.java b/backend/src/main/java/com/bakdata/conquery/apiv1/query/TableExportQuery.java index 236e762860..115fb6b28c 100644 --- a/backend/src/main/java/com/bakdata/conquery/apiv1/query/TableExportQuery.java +++ b/backend/src/main/java/com/bakdata/conquery/apiv1/query/TableExportQuery.java @@ -8,10 +8,10 @@ import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.OptionalInt; import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Consumer; +import java.util.function.Predicate; import java.util.stream.Collectors; import jakarta.validation.Valid; import jakarta.validation.constraints.NotEmpty; @@ -55,6 +55,7 @@ import lombok.NonNull; import lombok.RequiredArgsConstructor; import lombok.Setter; +import lombok.ToString; import lombok.extern.slf4j.Slf4j; @@ -72,6 +73,7 @@ @Slf4j @Getter @Setter +@ToString(onlyExplicitlyIncluded = false) @CPSType(id = "TABLE_EXPORT", base = QueryDescription.class) @RequiredArgsConstructor(onConstructor_ = {@JsonCreator}) public class TableExportQuery extends Query { @@ -79,17 +81,22 @@ public class TableExportQuery extends Query { @Valid @NotNull @NonNull + @ToString.Include protected final Query query; + @NotNull + @ToString.Include private Range dateRange = Range.all(); @NotEmpty @Valid + @ToString.Include private List tables; /** * @see TableExportQueryPlan#isRawConceptValues() */ + @ToString.Include private boolean rawConceptValues = true; /** @@ -121,13 +128,7 @@ public TableExportQueryPlan createQueryPlan(QueryPlanContext context) { } } - return new TableExportQueryPlan( - query.createQueryPlan(context), - CDateSet.create(CDateRange.of(dateRange)), - filterQueryNodes, - positions, - rawConceptValues - ); + return new TableExportQueryPlan(query.createQueryPlan(context), CDateSet.create(CDateRange.of(dateRange)), filterQueryNodes, positions, rawConceptValues); } @Override @@ -143,15 +144,6 @@ public void resolve(QueryResolveContext context) { // First is dates, second is source id final AtomicInteger currentPosition = new AtomicInteger(2); - secondaryIdPositions = calculateSecondaryIdPositions(currentPosition); - - final Set validityDates = tables.stream() - .map(CQConcept::getTables) - .flatMap(Collection::stream) - .map(CQTable::findValidityDate) - .filter(Objects::nonNull) - .collect(Collectors.toSet()); - // We need to know if a column is a concept column, so we can prioritize it, if it is also a SecondaryId conceptColumns = tables.stream() .map(CQConcept::getTables) @@ -162,18 +154,25 @@ public void resolve(QueryResolveContext context) { .filter(Objects::nonNull) .collect(Collectors.toSet()); - positions = calculateColumnPositions(currentPosition, tables, secondaryIdPositions, conceptColumns, validityDates); + secondaryIdPositions = calculateSecondaryIdPositions(currentPosition, conceptColumns, tables); + + final Set validityDates = + tables.stream().map(CQConcept::getTables).flatMap(Collection::stream).map(CQTable::findValidityDate).filter(Objects::nonNull).collect(Collectors.toSet()); + positions = calculateColumnPositions(currentPosition, tables, secondaryIdPositions, conceptColumns, validityDates); } - private Map calculateSecondaryIdPositions(AtomicInteger currentPosition) { + private static Map calculateSecondaryIdPositions( + AtomicInteger currentPosition, Set conceptColumns, List tables) { final Map secondaryIdPositions = new HashMap<>(); // SecondaryIds are pulled to the front and grouped over all tables tables.stream() .flatMap(con -> con.getTables().stream()) .flatMap(table -> Arrays.stream(table.getConnector().resolve().getResolvedTable().getColumns())) + // Concept Columns are placed separately so they won't provide a secondaryId + .filter(Predicate.not(conceptColumns::contains)) .map(Column::getSecondaryId) .filter(Objects::nonNull) .map(SecondaryIdDescriptionId::resolve) @@ -190,8 +189,7 @@ private static Map calculateColumnPositions( List tables, Map secondaryIdPositions, Collection conceptColumns, - Collection validityDates - ) { + Collection validityDates) { final Map positions = new HashMap<>(); @@ -232,11 +230,8 @@ public List getResultInfos() { private List createResultInfos(Set conceptColumns) { - OptionalInt max = positions.values().stream().mapToInt(i -> i).max(); - if (max.isEmpty()) { - throw new IllegalStateException("Unable to determine maximum position"); - } - final int size = max.getAsInt() + 1; + final int size = calculateWidth(positions); + ; final ResultInfo[] infos = new ResultInfo[size]; @@ -252,13 +247,12 @@ private List createResultInfos(Set conceptColumns) { } - final Map> connectorColumns = - tables.stream() - .flatMap(con -> con.getTables().stream()) - .map(CQTable::getConnector) - .map(ConnectorId::resolve) - .filter(con -> con.getColumn() != null) - .collect(Collectors.toMap(con -> con.getColumn().resolve(), Connector::getConcept)); + final Map> connectorColumns = tables.stream() + .flatMap(con -> con.getTables().stream()) + .map(CQTable::getConnector) + .map(ConnectorId::resolve) + .filter(con -> con.getColumn() != null) + .collect(Collectors.toMap(con -> con.getColumn().resolve(), Connector::getConcept)); for (Map.Entry entry : positions.entrySet()) { @@ -304,6 +298,10 @@ private List createResultInfos(Set conceptColumns) { return List.of(infos); } + public static int calculateWidth(Map positions) { + return positions.values().stream().max(Integer::compareTo).orElse(0) + 1; + } + @Override public void visit(Consumer visitor) { visitor.accept(this); diff --git a/backend/src/main/java/com/bakdata/conquery/models/query/queryplan/TableExportQueryPlan.java b/backend/src/main/java/com/bakdata/conquery/models/query/queryplan/TableExportQueryPlan.java index f81b79a312..1de21de4b8 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/query/queryplan/TableExportQueryPlan.java +++ b/backend/src/main/java/com/bakdata/conquery/models/query/queryplan/TableExportQueryPlan.java @@ -5,6 +5,7 @@ import java.util.Map; import java.util.Optional; +import com.bakdata.conquery.apiv1.query.TableExportQuery; import com.bakdata.conquery.apiv1.query.concept.filter.CQTable; import com.bakdata.conquery.models.common.CDateSet; import com.bakdata.conquery.models.common.daterange.CDateRange; @@ -69,7 +70,7 @@ public Optional execute(QueryExecutionContext ctx, Entity final List results = new ArrayList<>(); - final int totalColumns = positions.values().stream().mapToInt(i -> i).max().getAsInt() + 1; + final int totalColumns = TableExportQuery.calculateWidth(positions); final String entityId = entity.getId(); for (Map.Entry entry : tables.entrySet()) { diff --git a/backend/src/main/java/com/bakdata/conquery/sql/conversion/query/TableExportQueryConverter.java b/backend/src/main/java/com/bakdata/conquery/sql/conversion/query/TableExportQueryConverter.java index 83570651e4..c8013f36d6 100644 --- a/backend/src/main/java/com/bakdata/conquery/sql/conversion/query/TableExportQueryConverter.java +++ b/backend/src/main/java/com/bakdata/conquery/sql/conversion/query/TableExportQueryConverter.java @@ -39,6 +39,12 @@ @RequiredArgsConstructor public class TableExportQueryConverter implements NodeConverter { + /** + * Validity date is part of positions, but not when converting to SQL because it might have multiple columns and not just one. + * Thus, we need to apply an offset to the positions. + */ + private static final int POSITION_OFFSET = 1; + private final QueryStepTransformer queryStepTransformer; @Override @@ -49,27 +55,27 @@ public Class getConversionClass() { @Override public ConversionContext convert(TableExportQuery tableExportQuery, ConversionContext context) { - QueryStep convertedPrerequisite = convertPrerequisite(tableExportQuery, context); - Map positions = tableExportQuery.getPositions(); - CDateRange dateRestriction = CDateRange.of(tableExportQuery.getDateRange()); - - List convertedTables = tableExportQuery.getTables().stream() - .flatMap(concept -> concept.getTables().stream().map(table -> convertTable( - table, - concept, - dateRestriction, - convertedPrerequisite, - positions, - context - ))) - .toList(); - - QueryStep unionedTables = QueryStep.createUnionAllStep( + final QueryStep convertedPrerequisite = convertPrerequisite(tableExportQuery, context); + final Map positions = tableExportQuery.getPositions(); + final CDateRange dateRestriction = CDateRange.of(tableExportQuery.getDateRange()); + + final List convertedTables = tableExportQuery.getTables().stream() + .flatMap(concept -> concept.getTables().stream().map(table -> convertTable( + table, + concept, + dateRestriction, + convertedPrerequisite, + positions, + context + ))) + .toList(); + + final QueryStep unionedTables = QueryStep.createUnionAllStep( convertedTables, null, // no CTE name required as this step will be the final select List.of(convertedPrerequisite) ); - Select selectQuery = queryStepTransformer.toSelectQuery(unionedTables); + final Select selectQuery = queryStepTransformer.toSelectQuery(unionedTables); return context.withFinalQuery(new SqlQuery(selectQuery, tableExportQuery.getResultInfos())); } @@ -79,14 +85,14 @@ public ConversionContext convert(TableExportQuery tableExportQuery, ConversionCo */ private static QueryStep convertPrerequisite(TableExportQuery exportQuery, ConversionContext context) { - ConversionContext withConvertedPrerequisite = context.getNodeConversions().convert(exportQuery.getQuery(), context); + final ConversionContext withConvertedPrerequisite = context.getNodeConversions().convert(exportQuery.getQuery(), context); Preconditions.checkArgument(withConvertedPrerequisite.getQuerySteps().size() == 1, "Base query conversion should produce exactly 1 QueryStep"); - QueryStep convertedPrerequisite = withConvertedPrerequisite.getLastConvertedStep(); + final QueryStep convertedPrerequisite = withConvertedPrerequisite.getLastConvertedStep(); - Selects prerequisiteSelects = convertedPrerequisite.getQualifiedSelects(); - Selects selects = Selects.builder() - .ids(new SqlIdColumns(prerequisiteSelects.getIds().getPrimaryColumn())) - .build(); + final Selects prerequisiteSelects = convertedPrerequisite.getQualifiedSelects(); + final Selects selects = Selects.builder() + .ids(new SqlIdColumns(prerequisiteSelects.getIds().getPrimaryColumn())) + .build(); return QueryStep.builder() .cteName(FormCteStep.EXTRACT_IDS.getSuffix()) @@ -109,20 +115,22 @@ private static QueryStep convertTable( Map positions, ConversionContext context ) { - Field primaryColumn = TablePrimaryColumnUtil.findPrimaryColumn(cqTable.getConnector().resolve().getResolvedTable(), context.getConfig()); - SqlIdColumns ids = new SqlIdColumns(primaryColumn); - String conceptConnectorName = context.getNameGenerator().conceptConnectorName(concept, cqTable.getConnector().resolve(), context.getSqlPrintSettings().getLocale()); - Optional validityDate = convertTablesValidityDate(cqTable, conceptConnectorName, context); + final Field primaryColumn = TablePrimaryColumnUtil.findPrimaryColumn(cqTable.getConnector().resolve().getResolvedTable(), context.getConfig()); + final SqlIdColumns ids = new SqlIdColumns(primaryColumn); + final String conceptConnectorName = + context.getNameGenerator().conceptConnectorName(concept, cqTable.getConnector().resolve(), context.getSqlPrintSettings().getLocale()); + final Optional validityDate = convertTablesValidityDate(cqTable, conceptConnectorName, context); - List> exportColumns = initializeFields(cqTable, positions); - Selects selects = Selects.builder() - .ids(ids) - .validityDate(validityDate) - .sqlSelects(exportColumns) - .build(); + final List> exportColumns = initializeFields(cqTable, positions); - List filters = cqTable.getFilters().stream().map(filterValue -> filterValue.convertForTableExport(ids, context)).toList(); - Table joinedTable = joinConnectorTableWithPrerequisite(cqTable, ids, convertedPrerequisite, dateRestriction, context); + final Selects selects = Selects.builder() + .ids(ids) + .validityDate(validityDate) + .sqlSelects(exportColumns) + .build(); + + final List filters = cqTable.getFilters().stream().map(filterValue -> filterValue.convertForTableExport(ids, context)).toList(); + final Table joinedTable = joinConnectorTableWithPrerequisite(cqTable, ids, convertedPrerequisite, dateRestriction, context); return QueryStep.builder() .cteName(conceptConnectorName) @@ -136,72 +144,74 @@ private static Optional convertTablesValidityDate(CQTable table if (table.findValidityDate() == null) { return Optional.of(ColumnDateRange.empty()); } - SqlFunctionProvider functionProvider = context.getSqlDialect().getFunctionProvider(); - ColumnDateRange validityDate = functionProvider.forValidityDate(table.findValidityDate()); + final SqlFunctionProvider functionProvider = context.getSqlDialect().getFunctionProvider(); + final ColumnDateRange validityDate = functionProvider.forValidityDate(table.findValidityDate()); // when exporting tables, we want the validity date as a single-column daterange string expression straightaway - Field asStringExpression = functionProvider.encloseInCurlyBraces(functionProvider.daterangeStringExpression(validityDate)); + final Field asStringExpression = functionProvider.encloseInCurlyBraces(functionProvider.daterangeStringExpression(validityDate)); return Optional.of(ColumnDateRange.of(asStringExpression).asValidityDateRange(alias)); } private static List> initializeFields(CQTable cqTable, Map positions) { - Field[] exportColumns = createPlaceholders(positions, cqTable); + final Field[] exportColumns = createPlaceholders(positions); + + exportColumns[0] = createSourceInfoSelect(cqTable); + for (Column column : cqTable.getConnector().resolve().getResolvedTable().getColumns()) { // e.g. date column(s) are handled separately and not part of positions if (!positions.containsKey(column.getId())) { continue; } - int position = positions.get(column.getId()) - 1; + final int position = positions.get(column.getId()) - POSITION_OFFSET; exportColumns[position] = createColumnSelect(column, position); } return Arrays.stream(exportColumns).map(FieldWrapper::new).collect(Collectors.toList()); } - private static Field[] createPlaceholders(Map positions, CQTable cqTable) { + private static Table joinConnectorTableWithPrerequisite( + CQTable cqTable, + SqlIdColumns ids, + QueryStep convertedPrerequisite, + CDateRange dateRestriction, + ConversionContext context + ) { + final SqlFunctionProvider functionProvider = context.getSqlDialect().getFunctionProvider(); + final Table connectorTable = DSL.table(DSL.name(cqTable.getConnector().resolve().getResolvedTableId().getTable())); + final Table convertedPrerequisiteTable = DSL.table(DSL.name(convertedPrerequisite.getCteName())); - Field[] exportColumns = new Field[positions.size() + 1]; - exportColumns[0] = createSourceInfoSelect(cqTable); + final ColumnDateRange validityDate = functionProvider.forValidityDate(cqTable.findValidityDate()); + final List joinConditions = Stream.concat( + ids.join(convertedPrerequisite.getQualifiedSelects().getIds()).stream(), + Stream.of(functionProvider.dateRestriction(functionProvider.forCDateRange(dateRestriction), validityDate)) + ).toList(); + + return functionProvider.innerJoin(connectorTable, convertedPrerequisiteTable, joinConditions); + } + + private static Field[] createPlaceholders(Map positions) { + + final int size = TableExportQuery.calculateWidth(positions) - POSITION_OFFSET; + final Field[] exportColumns = new Field[size]; // if columns have the same computed position, they can share a common name because they will be unioned over multiple tables anyway - positions.forEach((column, position) -> { - int shifted = position - 1; - Field columnSelect = DSL.inline(null, Object.class).as("%s-%d".formatted(column.getColumn(), shifted)); - exportColumns[shifted] = columnSelect; - }); + for (int index = 0; index < exportColumns.length; index++) { + final Field columnSelect = DSL.inline(null, Object.class).as("null-%d".formatted(index)); + exportColumns[index] = columnSelect; + } return exportColumns; } private static Field createSourceInfoSelect(CQTable cqTable) { - String tableName = cqTable.getConnector().resolve().getResolvedTableId().getTable(); + final String tableName = cqTable.getConnector().resolve().getResolvedTableId().getTable(); return DSL.val(tableName).as(SharedAliases.SOURCE.getAlias()); } private static Field createColumnSelect(Column column, int position) { - String columnName = "%s-%s".formatted(column.getName(), position); + final String columnName = "%s-%s".formatted(column.getName(), position); return DSL.field(DSL.name(column.getTable().getName(), column.getName())) .as(columnName); } - private static Table joinConnectorTableWithPrerequisite( - CQTable cqTable, - SqlIdColumns ids, - QueryStep convertedPrerequisite, - CDateRange dateRestriction, - ConversionContext context - ) { - SqlFunctionProvider functionProvider = context.getSqlDialect().getFunctionProvider(); - Table connectorTable = DSL.table(DSL.name(cqTable.getConnector().resolve().getResolvedTableId().getTable())); - Table convertedPrerequisiteTable = DSL.table(DSL.name(convertedPrerequisite.getCteName())); - - ColumnDateRange validityDate = functionProvider.forValidityDate(cqTable.findValidityDate()); - List joinConditions = Stream.concat( - ids.join(convertedPrerequisite.getQualifiedSelects().getIds()).stream(), - Stream.of(functionProvider.dateRestriction(functionProvider.forCDateRange(dateRestriction), validityDate)) - ).toList(); - - return functionProvider.innerJoin(connectorTable, convertedPrerequisiteTable, joinConditions); - } - }