Refactor Jet join SQL generation for correct grouping

Refactored JetQuerySqlGenerator to properly group and parenthesize tables and joins, ensuring Jet/Access-compliant SQL when mixing cross joins, inner joins, and subqueries. Added helper methods for join grouping and alias extraction. Updated Northwind test assertions to match new Jet-style SQL output, improving compatibility and correctness. Minor refactoring of nullable numeric and type conversion handling.

Author: ChrisJollyAU

src/EFCore.Jet/Query/Sql/Internal/JetQuerySqlGenerator.cs

@@ -206,108 +206,199 @@ protected override Expression VisitSelect(SelectExpression selectExpression)
             return selectExpression;
         }
 
-        private void VisitJetTables(IReadOnlyList<TableExpressionBase> Tables, bool addfromsql, out List<ColumnExpression> colexp)
+        private void VisitJetTables(IReadOnlyList<TableExpressionBase> tables, bool addFromSql, out List<ColumnExpression> colexp)
         {
             colexp = [];
-            // Implement Jet's non-standard JOIN syntax and DUAL table workaround.
-            // TODO: This does not properly handle all cases (especially when cross joins are involved).
-            if (Tables.Any())
+            if (!tables.Any())
             {
-                if (addfromsql)
-                {
-                    Sql.AppendLine().Append("FROM ");
-                }
+                GeneratePseudoFromClause();
+                return;
+            }
 
-                const int maxTablesWithoutBrackets = 2;
+            if (addFromSql)
+                Sql.AppendLine().Append("FROM ");
 
-                var nonCrossTableCount = Tables.Count(t => t is not CrossJoinExpression and not CrossApplyExpression);
+            if (tables.Any(t => t is CrossJoinExpression or CrossApplyExpression))
+                VisitJetTablesGrouped(tables, colexp);
+            else
+                VisitJetTablesLinear(tables, colexp);
+        }
 
-                Sql.Append(
-                    new string(
-                        '(',
-                        Math.Max(0, nonCrossTableCount - maxTablesWithoutBrackets)));
+        // No cross joins: existing linear parenthesization algorithm.
+        private void VisitJetTablesLinear(IReadOnlyList<TableExpressionBase> tables, List<ColumnExpression> colexp)
+        {
+            const int maxTablesWithoutBrackets = 2;
+            var nonCrossTableCount = tables.Count(t => t is not CrossJoinExpression and not CrossApplyExpression);
+            Sql.Append(new string('(', Math.Max(0, nonCrossTableCount - maxTablesWithoutBrackets)));
 
-                var nonCrossTablesSeen = 0;
+            var nonCrossTablesSeen = 0;
+            for (var index = 0; index < tables.Count; index++)
+            {
+                var tableExpression = tables[index];
+                var isCrossExpression = tableExpression is CrossJoinExpression or CrossApplyExpression;
 
-                for (var index = 0; index < Tables.Count; index++)
+                if (tableExpression is CrossApplyExpression or OuterApplyExpression)
+                    throw new UnreachableException();
+
+                if (!isCrossExpression)
+                    nonCrossTablesSeen++;
+
+                if (index > 0)
                 {
-                    var tableExpression = Tables[index];
+                    if (isCrossExpression)
+                        Sql.Append(",");
+                    else if (nonCrossTablesSeen > maxTablesWithoutBrackets)
+                        Sql.Append(")");
 
-                    var isApplyExpression = tableExpression is CrossApplyExpression or OuterApplyExpression;
-                    var isCrossExpression = tableExpression is CrossJoinExpression or CrossApplyExpression;
-                    var isNonCrossExpression = !isCrossExpression;
+                    Sql.AppendLine();
+                }
 
-                    if (isApplyExpression)
+                if (tableExpression is InnerJoinExpression innerJoin)
+                {
+                    var tempcolexp = innerJoin.JoinPredicate switch
                     {
-                        throw new UnreachableException();
-                    }
+                        SqlBinaryExpression bin => ExtractColumnExpressions(bin),
+                        SqlUnaryExpression unary => ExtractColumnExpressions(unary),
+                        _ => (List<ColumnExpression>)[]
+                    };
 
-                    if (isNonCrossExpression)
+                    if (tempcolexp.Any(col => col.TableAlias == tables[0].Alias))
                     {
-                        nonCrossTablesSeen++;
+                        Visit(tableExpression);
                     }
-
-                    if (index > 0)
+                    else
                     {
-                        if (isCrossExpression)
-                        {
-                            Sql.Append(",");
-                        }
-                        else if (nonCrossTablesSeen > maxTablesWithoutBrackets)
-                        {
-                            Sql.Append(")");
-                        }
-
-                        Sql.AppendLine();
+                        colexp.AddRange(tempcolexp);
+                        Sql.Append("LEFT JOIN ");
+                        Visit(innerJoin.Table);
+                        Sql.Append(" ON ");
+                        Visit(innerJoin.JoinPredicate);
                     }
+                }
+                else
+                {
+                    Visit(tableExpression);
+                }
+            }
+        }
 
-                    List<ColumnExpression> tempcolexp;
-                    if (tableExpression is InnerJoinExpression expression)
-                    {
-                        if (expression.JoinPredicate is SqlBinaryExpression binaryJoin)
-                        {
-                            tempcolexp = ExtractColumnExpressions(binaryJoin);
-                        }
-                        else if (expression.JoinPredicate is SqlUnaryExpression unaryJoin)
+        // Cross joins present: group each primary table with its associated joins so that
+        // each group is parenthesized independently before being comma-cross-joined.
+        // Jet rejects mixing comma (cross-join) and explicit JOIN at the same paren level.
+        private void VisitJetTablesGrouped(IReadOnlyList<TableExpressionBase> tables, List<ColumnExpression> colexp)
+        {
+            var groups = new List<(TableExpressionBase Primary, List<PredicateJoinExpressionBase> Joins, HashSet<string> OwnedAliases)>();
+
+            foreach (var table in tables)
+            {
+                switch (table)
+                {
+                    case CrossApplyExpression or OuterApplyExpression:
+                        throw new UnreachableException();
+                    case CrossJoinExpression cj:
+                        groups.Add((cj.Table, [], new HashSet<string> { cj.Table.Alias! }));
+                        break;
+                    case PredicateJoinExpressionBase join:
+                        var predicateAliases = ExtractTableAliases(join.JoinPredicate);
+                        var owner = groups.FirstOrDefault(g => g.OwnedAliases.Overlaps(predicateAliases));
+                        if (owner.Primary != null)
                         {
-                            tempcolexp = ExtractColumnExpressions(unaryJoin);
+                            owner.Joins.Add(join);
+                            owner.OwnedAliases.Add(join.Table.Alias!);
                         }
                         else
                         {
-                            tempcolexp = [];
+                            var last = groups[^1];
+                            last.Joins.Add(join);
+                            last.OwnedAliases.Add(join.Table.Alias!);
                         }
+                        break;
+                    default:
+                        groups.Add((table, [], new HashSet<string> { table.Alias! }));
+                        break;
+                }
+            }
 
-                        var referencesFirstTable = false;
-                        foreach (var col in tempcolexp)
-                        {
-                            if (col.TableAlias == Tables[0].Alias)
-                            {
-                                referencesFirstTable = true;
-                                break;
-                            }
-                        }
+            for (var i = 0; i < groups.Count; i++)
+            {
+                if (i > 0)
+                    Sql.Append(",").AppendLine();
 
-                        if (referencesFirstTable)
-                        {
-                            Visit(tableExpression);
-                            continue;
-                        }
+                var (primary, joins, _) = groups[i];
+                var groupTableCount = 1 + joins.Count;
 
-                        colexp.AddRange(tempcolexp);
-                        Sql.Append("LEFT JOIN ");
-                        Visit(expression.Table);
-                        Sql.Append(" ON ");
-                        Visit(expression.JoinPredicate);
-                    }
-                    else
-                    {
-                        Visit(tableExpression);
-                    }
+                if (joins.Count > 0)
+                    Sql.Append("(");
+
+                Sql.Append(new string('(', Math.Max(0, groupTableCount - 2)));
+
+                Visit(primary);
+
+                var nonSeen = 1;
+                foreach (var join in joins)
+                {
+                    nonSeen++;
+                    if (nonSeen > 2)
+                        Sql.Append(")");
+
+                    Sql.AppendLine();
+                    EmitJoinInGroup(join, primary, colexp);
+                }
+
+                if (joins.Count > 0)
+                    Sql.Append(")");
+            }
+        }
+
+        private void EmitJoinInGroup(PredicateJoinExpressionBase join, TableExpressionBase groupPrimary, List<ColumnExpression> colexp)
+        {
+            if (join is InnerJoinExpression innerJoin)
+            {
+                var tempcolexp = innerJoin.JoinPredicate switch
+                {
+                    SqlBinaryExpression bin => ExtractColumnExpressions(bin),
+                    SqlUnaryExpression unary => ExtractColumnExpressions(unary),
+                    _ => (List<ColumnExpression>)[]
+                };
+
+                if (tempcolexp.Any(col => col.TableAlias == groupPrimary.Alias))
+                    Visit(join);
+                else
+                {
+                    colexp.AddRange(tempcolexp);
+                    Sql.Append("LEFT JOIN ");
+                    Visit(innerJoin.Table);
+                    Sql.Append(" ON ");
+                    Visit(innerJoin.JoinPredicate);
                 }
             }
             else
             {
-                GeneratePseudoFromClause();
+                Visit(join);
+            }
+        }
+
+        private HashSet<string> ExtractTableAliases(SqlExpression expression)
+        {
+            var result = new HashSet<string>();
+            CollectTableAliases(expression, result);
+            return result;
+        }
+
+        private static void CollectTableAliases(SqlExpression expression, HashSet<string> result)
+        {
+            switch (expression)
+            {
+                case ColumnExpression col:
+                    result.Add(col.TableAlias);
+                    break;
+                case SqlBinaryExpression bin:
+                    CollectTableAliases(bin.Left, result);
+                    CollectTableAliases(bin.Right, result);
+                    break;
+                case SqlUnaryExpression unary:
+                    CollectTableAliases(unary.Operand, result);
+                    break;
             }
         }
 
@@ -369,15 +460,25 @@ protected override Expression VisitColumn(ColumnExpression columnExpression)
             if (columnExpression.IsNullable && _nullNumerics.Contains(columnExpression.Name) && _convertMappings.TryGetValue(columnExpression.Type.Name, out var function))
             {
 
+                bool useValCStrCol = false;//columnExpression.Type.Name is nameof(Decimal) or nameof(Int64);
                 if (parent.TryPeek(out var exp) && exp is SqlBinaryExpression)
                 {
                     Sql.Append("IIF(");
                     base.VisitColumn(columnExpression);
                     Sql.Append(" IS NULL, NULL, ");
-                    Sql.Append(function);
-                    Sql.Append("(");
-                    base.VisitColumn(columnExpression);
-                    Sql.Append(")");
+                    if (useValCStrCol)
+                    {
+                        Sql.Append("Val(CStr(");
+                        base.VisitColumn(columnExpression);
+                        Sql.Append("))");
+                    }
+                    else
+                    {
+                        Sql.Append(function);
+                        Sql.Append("(");
+                        base.VisitColumn(columnExpression);
+                        Sql.Append(")");
+                    }
                     Sql.Append(")");
                     return columnExpression;
                 }
@@ -501,9 +602,18 @@ protected override Expression VisitSqlParameter(SqlParameterExpression sqlParame
                 {
                     if (_convertMappings.TryGetValue(sqlParameterExpression.Type.Name, out var conv))
                     {
-                        Sql.Append($"{conv}(");
-                        base.VisitSqlParameter(sqlParameterExpression);
-                        Sql.Append(")");
+                        /*if (sqlParameterExpression.Type.Name is nameof(Decimal) or nameof(Int64))
+                        {
+                            Sql.Append("Val(CStr(");
+                            base.VisitSqlParameter(sqlParameterExpression);
+                            Sql.Append("))");
+                        }
+                        else*/
+                        {
+                            Sql.Append($"{conv}(");
+                            base.VisitSqlParameter(sqlParameterExpression);
+                            Sql.Append(")");
+                        }
                         return sqlParameterExpression;
                     }
                 }
@@ -625,6 +735,21 @@ private Expression VisitJetConvertExpression(SqlUnaryExpression convertExpressio
                 SqlExpression checksqlexp = convertExpression.Operand;
                 SqlExpression? notnullsqlexp = null;
 
+                bool useValCStr = false;//convertExpression.Type.Name is nameof(Decimal) or nameof(Int64);
+
+                /*SqlFunctionExpression WrapConvert(SqlExpression inner) =>
+                    useValCStr
+                        ? new SqlFunctionExpression("Val",
+                            [new SqlFunctionExpression("CStr", [inner], false, [false], typeof(string), null)],
+                            false, [false], typeMapping.ClrType, null)
+                        : new SqlFunctionExpression(function, [inner], false, [false], typeMapping.ClrType, null);*/
+
+                SqlFunctionExpression WrapConvert(SqlExpression inner) =>
+                    useValCStr
+                        ? new SqlFunctionExpression("CVar", [inner],
+                            false, [false], typeMapping.ClrType, null)
+                        : new SqlFunctionExpression(function, [inner], false, [false], typeMapping.ClrType, null);
+
                 if (convertExpression.TypeMapping is ByteArrayTypeMapping)
                 {
                     notnullsqlexp = checksqlexp;
@@ -636,8 +761,7 @@ private Expression VisitJetConvertExpression(SqlUnaryExpression convertExpressio
                         if (convertExpression.Type == typeof(bool))
                         {
                             // bool?bool: no flip needed, CBOOL(x) correctly returns true for any non-zero
-                            notnullsqlexp = new SqlFunctionExpression(function, [convertExpression.Operand],
-                                false, [false], typeMapping.ClrType, null);
+                            notnullsqlexp = WrapConvert(convertExpression.Operand);
                         }
                         else
                         {
@@ -650,14 +774,12 @@ private Expression VisitJetConvertExpression(SqlUnaryExpression convertExpressio
                                 convertExpression.Operand.Type,
                                 convertExpression.Operand.TypeMapping);
 
-                            notnullsqlexp = new SqlFunctionExpression(function, [flippedOperand],
-                                false, [false], typeMapping.ClrType, null);
+                            notnullsqlexp = WrapConvert(flippedOperand);
                         }
                     }
                     else
                     {
-                        notnullsqlexp = new SqlFunctionExpression(function, [convertExpression.Operand],
-                            false, [false], typeMapping.ClrType, null);
+                        notnullsqlexp = WrapConvert(convertExpression.Operand);
                     }
                 }
 

test/EFCore.Jet.FunctionalTests/Query/NorthwindJoinQueryJetTest.cs

@@ -1039,14 +1039,14 @@ public override async Task GroupJoin_subquery_projection_outer_mixed(bool async)
 
             AssertSql(
                 """
-SELECT [c].[CustomerID] AS [A], [t].[CustomerID] AS [B], [o0].[CustomerID] AS [C]
-FROM [Customers] AS [c]
-CROSS JOIN (
-    SELECT TOP(1) [o].[CustomerID]
-    FROM [Orders] AS [o]
-    ORDER BY [o].[OrderID]
-) AS [t]
-INNER JOIN [Orders] AS [o0] ON [c].[CustomerID] = [o0].[CustomerID]
+SELECT `c`.`CustomerID` AS `A`, `o0`.`CustomerID` AS `B`, `o1`.`CustomerID` AS `C`
+FROM (`Customers` AS `c`
+INNER JOIN `Orders` AS `o1` ON `c`.`CustomerID` = `o1`.`CustomerID`),
+(
+    SELECT TOP 1 `o`.`CustomerID`
+    FROM `Orders` AS `o`
+    ORDER BY `o`.`OrderID`
+) AS `o0`
 """);
         }