Skip to content

Commit

Permalink
#5179 Fixed results returned from query joining the same table twice.
Browse files Browse the repository at this point in the history
  • Loading branch information
pawelsalawa committed Dec 22, 2024
1 parent 4e74c32 commit ba9b48c
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 70 deletions.
4 changes: 4 additions & 0 deletions ChangeLog.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# ChangeLog

### 3.4.12
- BIGFIX: #5179 Fixed results returned from query joining the same table twice.
- BIGFIX: #5179 Fixed smart execution when joining tables with USING clause, so the result metadata is extracted properly and results can be edited.

### 3.4.11
- CHANGE: SQLite updated to 3.47.2
- BUGFIX: #5161 An ultimate fix for dialog windows positioning, so they no longer can appear outside of visible screen.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ QHash<SelectResolver::Table,QHash<QString,QString>> QueryExecutorAddRowIds::addR
return rowIdColsMap;

}
core->rebuildTokens();
select->rebuildTokens();

// Getting all tables we need to get ROWID for
SelectResolver resolver(db, select->tokens.detokenize(), context->dbNameToAttach);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,15 @@ bool QueryExecutorColumns::exec()
context->resultColumns << resultColumn; // store it in context for later usage by any step
}

// qDebug() << "before: " << context->processedQuery;
//qDebug() << "before: " << context->processedQuery;
// Update query
select->rebuildTokens();
wrapWithAliasedColumns(select.data());
// #5179 does not seem to be needed anymore, because query executor alias is applied always in the main column loop above.
// Keeping the commented reference here for a while, but to be removed in future (due to end of 2025).
//wrapWithAliasedColumns(select.data());
updateQueries();

// qDebug() << "after: " << context->processedQuery;
//qDebug() << "after: " << context->processedQuery;

return true;
}
Expand Down Expand Up @@ -176,11 +178,13 @@ SqliteSelect::Core::ResultColumn* QueryExecutorColumns::getResultColumnForSelect
}
}

selectResultColumn->asKw = true;
if (!col.alias.isNull())
selectResultColumn->alias = col.alias;
else
selectResultColumn->alias = resultColumn->queryExecutorAlias;
selectResultColumn->expr->column = col.alias;

// #5179 duplicate of the same source table columns (but with different table alias) requires executor alias to be applied
// always and immediately here to get proper results.
selectResultColumn->asKw = true;
selectResultColumn->alias = resultColumn->queryExecutorAlias;

// If this alias was already used we need to use sequential alias
static_qstring(aliasTpl, "%1:%2");
Expand Down Expand Up @@ -212,63 +216,6 @@ bool QueryExecutorColumns::isRowIdColumnAlias(const QString& alias)
return false;
}

void QueryExecutorColumns::wrapWithAliasedColumns(SqliteSelect* select)
{
// Wrap everything in a surrounding SELECT and given query executor alias to all columns this time
TokenList sepTokens;
sepTokens << TokenPtr::create(Token::OPERATOR, ",") << TokenPtr::create(Token::SPACE, " ");

bool first = true;
TokenList outerColumns;
QStringList columnNamesUsed;
QString baseColName;
QString colName;
static_qstring(colNameTpl, "%1:%2");
for (QueryExecutor::ResultColumnPtr& resCol : context->resultColumns)
{
if (!first)
outerColumns += sepTokens;

// If alias was given, we use it. If it was anything but expression, we also use its display name,
// because it's explicit column (no matter if from table, or table alias).
baseColName = QString();
if (!resCol->queryExecutorAlias.isNull())
baseColName = resCol->alias;
else if (!resCol->expression)
baseColName = resCol->column;

if (!baseColName.isNull())
{
colName = baseColName;
for (int i = 1; columnNamesUsed.contains(colName, Qt::CaseInsensitive); i++)
colName = colNameTpl.arg(resCol->column, QString::number(i));

columnNamesUsed << colName;
outerColumns << TokenPtr::create(Token::OTHER, wrapObjIfNeeded(colName));
outerColumns << TokenPtr::create(Token::SPACE, " ");
outerColumns << TokenPtr::create(Token::KEYWORD, "AS");
outerColumns << TokenPtr::create(Token::SPACE, " ");
}
outerColumns << TokenPtr::create(Token::OTHER, resCol->queryExecutorAlias);
first = false;
}

for (QueryExecutor::ResultRowIdColumnPtr& rowIdColumn : context->rowIdColumns)
{
for (QString& alias : rowIdColumn->queryExecutorAliasToColumn.keys())
{
if (!first)
outerColumns += sepTokens;

outerColumns << TokenPtr::create(Token::OTHER, alias);
first = false;
}
}

//QString t = outerColumns.detokenize(); // keeping it for debug purposes
select->tokens = wrapSelect(select->tokens, outerColumns);
}

bool QueryExecutorColumns::isRowIdColumn(const QString& columnAlias)
{
// In case of "SELECT * FROM (SELECT * FROM test);" the SelectResolver will return ROWID columns twice for each table listed,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ class QueryExecutorColumns : public QueryExecutorStep
*/
bool isRowIdColumnAlias(const QString& alias);

void wrapWithAliasedColumns(SqliteSelect* select);
bool isRowIdColumn(const QString& columnAlias);
QStringList rowIdColNames;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

bool QueryExecutorExecute::exec()
{
// qDebug() << "q:" << context->processedQuery;
//qDebug() << "q:" << context->processedQuery;

startTime = QDateTime::currentMSecsSinceEpoch();
return executeQueries();
Expand Down
22 changes: 20 additions & 2 deletions SQLiteStudio3/coreSQLiteStudio/selectresolver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,19 @@ QList<SelectResolver::Column> SelectResolver::resolveSingleSourceSubSelect(Sqlit

QList<SelectResolver::Column> SelectResolver::resolveOtherSource(SqliteSelect::Core::JoinSourceOther *otherSrc)
{
return resolveSingleSource(otherSrc->singleSource);
QList<SelectResolver::Column> joinedColumns = resolveSingleSource(otherSrc->singleSource);
if (!otherSrc->joinConstraint || otherSrc->joinConstraint->expr)
return joinedColumns;

// Skip right-hand (aka "other") source column if it matches any of names listed in USING clause.
QSet<QString> usingColumns;
for (QString& colName : otherSrc->joinConstraint->columnNames)
usingColumns << colName.toLower();

return filter<SelectResolver::Column>(joinedColumns, [usingColumns](const SelectResolver::Column& col)
{
return !usingColumns.contains((col.alias.isNull() ? col.column : col.alias).toLower());
});
}

QList<SelectResolver::Column> SelectResolver::resolveSubSelect(SqliteSelect *select)
Expand Down Expand Up @@ -761,9 +773,15 @@ QList<SelectResolver::Column> SelectResolver::resolveSubSelect(SqliteSelect *sel
}
else
{
static_qstring(colTpl, "%1.%2 AS %3");
auto fn = [](const Column& c) {return colTpl.arg(c.table, c.column, c.alias);};
QStringList resolverColumns = map<Column, QString>(columnSourcesFromInternal, fn);
QStringList sqliteColumns = map<Column, QString>(columnSources, fn);
qCritical() << "Number of columns resolved by internal SchemaResolver is different than resolved by SQLite API:"
<< columnSourcesFromInternal.size() << "!=" << columnSources.size()
<< ", therefore table alias may be identified incorrectly (from resolver, but not by SQLite API)";
<< ", therefore table alias may be identified incorrectly (from resolver, but not by SQLite API)"
<< ". Columns were resolved from query:" << query << ". Colums resolved by SchemaResolver:"
<< resolverColumns.join(", ") << ", while columns from SQLite:" << sqliteColumns.join(", ");
}

if (compound)
Expand Down
2 changes: 1 addition & 1 deletion SQLiteStudio3/coreSQLiteStudio/sqlitestudio.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@

DEFINE_SINGLETON(SQLiteStudio)

static const int sqlitestudioVersion = 30411;
static const int sqlitestudioVersion = 30412;

SQLiteStudio::SQLiteStudio()
{
Expand Down

0 comments on commit ba9b48c

Please sign in to comment.