From 4238282d1dc188e639882db8509cc13668674e7f Mon Sep 17 00:00:00 2001 From: sonika-shah <58761340+sonika-shah@users.noreply.github.com> Date: Fri, 3 Jul 2026 14:23:02 +0530 Subject: [PATCH 1/3] fix(column-bulk): sort column grid results alphabetically by column name (#29699) * fix(column-bulk): sort column grid results alphabetically by column name The column grid rendered rows in HashMap iteration order because ColumnMetadataGrouper.groupColumns emitted them in entrySet order without sorting, discarding the ascending order the aggregators request from the search engine. Sort gridItems by columnName (case-insensitive) so the grid is deterministic and alphabetical across both engines and all query paths. Fixes #29698 * test/fix: deterministic tie-breaker + robust ordering tests Address review feedback on the column grid sort: - Add a case-sensitive tie-breaker (.thenComparing) so column names differing only in case have a stable order instead of falling back to map iteration. - Use LinkedHashMap with deliberately non-sorted insertion in the sort tests so the unsorted input is deterministic (not reliant on HashMap iteration order). - Add a test asserting case-only collisions sort deterministically. * docs: clarify sort comment does not assert caller map type * docs: tighten sort comment and make test comments generic * docs: reword sort comment to say iteration order is not guaranteed --- .../service/search/ColumnMetadataGrouper.java | 9 ++ .../search/ColumnMetadataGrouperTest.java | 84 +++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/search/ColumnMetadataGrouper.java b/openmetadata-service/src/main/java/org/openmetadata/service/search/ColumnMetadataGrouper.java index 0ebc13ef4756..23ea61d2b669 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/search/ColumnMetadataGrouper.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/search/ColumnMetadataGrouper.java @@ -16,6 +16,7 @@ import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import java.util.ArrayList; +import java.util.Comparator; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -101,6 +102,14 @@ public static List groupColumns( gridItems.add(gridItem); } + // groupColumns is the single sink for every query path. The input map's iteration order is not + // guaranteed (current callers build it as a HashMap), so sort here for a stable alphabetical + // order. Case-insensitive for natural listing, with a case-sensitive tie-breaker so names that + // differ only in case (e.g. "Name"/"name") stay stable instead of relying on map order. + gridItems.sort( + Comparator.comparing(ColumnGridItem::getColumnName, String.CASE_INSENSITIVE_ORDER) + .thenComparing(ColumnGridItem::getColumnName)); + return gridItems; } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/search/ColumnMetadataGrouperTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/search/ColumnMetadataGrouperTest.java index fefc8bcb64d9..25986c8ce3a0 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/search/ColumnMetadataGrouperTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/search/ColumnMetadataGrouperTest.java @@ -20,6 +20,7 @@ import java.util.ArrayList; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import org.junit.jupiter.api.Test; @@ -337,4 +338,87 @@ void testGroupColumns_emptyMap() { assertNotNull(result); assertEquals(0, result.size()); } + + @Test + void testGroupColumns_resultsAreSortedAscendingByColumnName() { + // Feed columns in a non-alphabetical order (LinkedHashMap keeps the input order deterministic) + // and verify the grid is returned sorted ascending by column name. + Map> columnsByName = + new LinkedHashMap<>(); + List scrambled = + List.of( + "c_admpkg", + "c_addin", + "c_active_agent_id", + "c_admincare", + "c_address", + "c_admission_date", + "c_acasclop", + "c_abnorm", + "c_abbrevation"); + for (String name : scrambled) { + columnsByName.put(name, singleOccurrence(name)); + } + + List result = ColumnMetadataGrouper.groupColumns(columnsByName); + + List actual = result.stream().map(ColumnGridItem::getColumnName).toList(); + List expected = new ArrayList<>(scrambled); + expected.sort(String.CASE_INSENSITIVE_ORDER); + assertEquals(expected, actual); + assertEquals("c_abbrevation", actual.get(0)); + assertEquals("c_abnorm", actual.get(1)); + } + + @Test + void testGroupColumns_sortIsCaseInsensitive() { + // Insertion order (Zebra, apple, Mango) is non-alphabetical and preserved by LinkedHashMap. + Map> columnsByName = + new LinkedHashMap<>(); + for (String name : List.of("Zebra", "apple", "Mango")) { + columnsByName.put(name, singleOccurrence(name)); + } + + List result = ColumnMetadataGrouper.groupColumns(columnsByName); + + List actual = result.stream().map(ColumnGridItem::getColumnName).toList(); + assertEquals(List.of("apple", "Mango", "Zebra"), actual); + } + + @Test + void testGroupColumns_caseOnlyCollisionsAreDeterministic() { + // Names differing only in case are equal under CASE_INSENSITIVE_ORDER, so the case-sensitive + // tie-breaker decides their order. Insert in reverse of the expected result to prove the sort, + // not the map, determines it. + Map> columnsByName = + new LinkedHashMap<>(); + for (String name : List.of("name", "Name")) { + columnsByName.put(name, singleOccurrence(name)); + } + + List result = ColumnMetadataGrouper.groupColumns(columnsByName); + + List actual = result.stream().map(ColumnGridItem::getColumnName).toList(); + // Natural String order: 'N' (0x4E) precedes 'n' (0x6E). + assertEquals(List.of("Name", "name"), actual); + } + + private static List singleOccurrence(String columnName) { + Column column = new Column(); + column.setName(columnName); + column.setDataType(ColumnDataType.STRING); + column.setFullyQualifiedName("sample_data.db.schema.table1." + columnName); + + List occurrences = new ArrayList<>(); + occurrences.add( + new ColumnMetadataGrouper.ColumnWithContext( + column, + "table", + "sample_data.db.schema.table1", + "Table 1", + "sample_data", + "db", + "schema")); + return occurrences; + } } From 7a80b37643892c512818441b5094728b7741569d Mon Sep 17 00:00:00 2001 From: sonika-shah <58761340+sonika-shah@users.noreply.github.com> Date: Sun, 5 Jul 2026 23:57:36 +0530 Subject: [PATCH 2/3] fix(column-bulk): make grid sort null-safe for null column names Guard the column-name comparator with Comparator.nullsLast so a null column name (e.g. a null map key) sorts last instead of throwing an NPE. Adds a test covering the null case. --- .../service/search/ColumnMetadataGrouper.java | 7 +++++-- .../search/ColumnMetadataGrouperTest.java | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/search/ColumnMetadataGrouper.java b/openmetadata-service/src/main/java/org/openmetadata/service/search/ColumnMetadataGrouper.java index 23ea61d2b669..f7ab13bc02f3 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/search/ColumnMetadataGrouper.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/search/ColumnMetadataGrouper.java @@ -106,9 +106,12 @@ public static List groupColumns( // guaranteed (current callers build it as a HashMap), so sort here for a stable alphabetical // order. Case-insensitive for natural listing, with a case-sensitive tie-breaker so names that // differ only in case (e.g. "Name"/"name") stay stable instead of relying on map order. + // nullsLast guards against a null column name (e.g. a null map key) rather than throwing. gridItems.sort( - Comparator.comparing(ColumnGridItem::getColumnName, String.CASE_INSENSITIVE_ORDER) - .thenComparing(ColumnGridItem::getColumnName)); + Comparator.comparing( + ColumnGridItem::getColumnName, Comparator.nullsLast(String.CASE_INSENSITIVE_ORDER)) + .thenComparing( + ColumnGridItem::getColumnName, Comparator.nullsLast(Comparator.naturalOrder()))); return gridItems; } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/search/ColumnMetadataGrouperTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/search/ColumnMetadataGrouperTest.java index 25986c8ce3a0..44e7c9e92b63 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/search/ColumnMetadataGrouperTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/search/ColumnMetadataGrouperTest.java @@ -19,6 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; @@ -403,6 +404,21 @@ void testGroupColumns_caseOnlyCollisionsAreDeterministic() { assertEquals(List.of("Name", "name"), actual); } + @Test + void testGroupColumns_nullColumnNameSortsLastWithoutError() { + // A null column name (e.g. a null map key) must not throw; it sorts last (nullsLast). + Map> columnsByName = + new LinkedHashMap<>(); + columnsByName.put("beta", singleOccurrence("beta")); + columnsByName.put(null, singleOccurrence(null)); + columnsByName.put("alpha", singleOccurrence("alpha")); + + List result = ColumnMetadataGrouper.groupColumns(columnsByName); + + List actual = result.stream().map(ColumnGridItem::getColumnName).toList(); + assertEquals(Arrays.asList("alpha", "beta", null), actual); + } + private static List singleOccurrence(String columnName) { Column column = new Column(); column.setName(columnName); From 161ec259879b58ae855333fdb09d84068e5b66b2 Mon Sep 17 00:00:00 2001 From: sonika-shah <58761340+sonika-shah@users.noreply.github.com> Date: Mon, 6 Jul 2026 15:29:16 +0530 Subject: [PATCH 3/3] fix(column-bulk): skip null-named columns instead of emitting a null required field A null column name (columnName is required in the column-grid response) is now skipped when building rows, so a malformed bucket is dropped and the remaining valid columns are returned. This also avoids the sort NPE at the source; the comparator keeps nullsLast as a backstop. Test updated to assert the null-named row is dropped. --- .../service/search/ColumnMetadataGrouper.java | 5 +++++ .../service/search/ColumnMetadataGrouperTest.java | 8 ++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/search/ColumnMetadataGrouper.java b/openmetadata-service/src/main/java/org/openmetadata/service/search/ColumnMetadataGrouper.java index f7ab13bc02f3..213717b32935 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/search/ColumnMetadataGrouper.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/search/ColumnMetadataGrouper.java @@ -40,6 +40,11 @@ public static List groupColumns( for (Map.Entry> entry : columnsByName.entrySet()) { String columnName = entry.getKey(); + if (columnName == null) { + // A column with no name isn't a valid grid row (columnName is required in the response); + // skip the malformed bucket so we return the remaining valid columns. + continue; + } List occurrences = entry.getValue(); Map groups = new HashMap<>(); diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/search/ColumnMetadataGrouperTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/search/ColumnMetadataGrouperTest.java index 44e7c9e92b63..f495413a09ab 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/search/ColumnMetadataGrouperTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/search/ColumnMetadataGrouperTest.java @@ -19,7 +19,6 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.ArrayList; -import java.util.Arrays; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; @@ -405,8 +404,9 @@ void testGroupColumns_caseOnlyCollisionsAreDeterministic() { } @Test - void testGroupColumns_nullColumnNameSortsLastWithoutError() { - // A null column name (e.g. a null map key) must not throw; it sorts last (nullsLast). + void testGroupColumns_nullColumnNameIsSkipped() { + // A null column name (e.g. a null map key) is dropped, not returned with a null required field, + // and must not throw. The remaining valid columns still come back sorted. Map> columnsByName = new LinkedHashMap<>(); columnsByName.put("beta", singleOccurrence("beta")); @@ -416,7 +416,7 @@ void testGroupColumns_nullColumnNameSortsLastWithoutError() { List result = ColumnMetadataGrouper.groupColumns(columnsByName); List actual = result.stream().map(ColumnGridItem::getColumnName).toList(); - assertEquals(Arrays.asList("alpha", "beta", null), actual); + assertEquals(List.of("alpha", "beta"), actual); } private static List singleOccurrence(String columnName) {