ES|QL|DS: fix partition_path literal binding (#158510)

Assisted by Cursor/Claude/Copilot

Author: julian-elastic

docs/changelog/158510.yaml

@@ -0,0 +1,5 @@
+pr: 158510
+summary: Fix partition_path literal binding and reject templates that cannot name distinct columns
+area: ES|QL
+type: bug
+issues: []

x-pack/plugin/esql-datasource-s3/src/test/java/org/elasticsearch/xpack/esql/datasource/s3/S3DataSourceValidatorTests.java

@@ -641,11 +641,38 @@ public void testValidateDatasetErrorBudgetConflictsWithFailFast() {
 
     public void testValidateDatasetPartitionPath() {
         assertEquals(
-            "year=*/month=*",
-            validator.validateDataset(Map.of(), "s3://b/p", Map.of("partition_path", "year=*/month=*")).get("partition_path")
+            "{year}/{month}",
+            validator.validateDataset(Map.of(), "s3://b/p", Map.of("partition_path", "{year}/{month}")).get("partition_path")
         );
     }
 
+    public void testValidateDatasetRejectsPlaceholderlessPartitionPath() {
+        ValidationException e = expectThrows(
+            ValidationException.class,
+            () -> validator.validateDataset(Map.of(), "s3://b/p", Map.of("partition_path", "year={year}"))
+        );
+        assertThat(e.getMessage(), containsString("partition_path"));
+        assertThat(e.getMessage(), containsString("{name}"));
+    }
+
+    public void testValidateDatasetRejectsGlobShapedPartitionPath() {
+        ValidationException e = expectThrows(
+            ValidationException.class,
+            () -> validator.validateDataset(Map.of(), "s3://b/p", Map.of("partition_path", "{year}/{month}/*.csv"))
+        );
+        assertThat(e.getMessage(), containsString("partition_path"));
+        assertThat(e.getMessage(), containsString("*.csv"));
+    }
+
+    public void testValidateDatasetRejectsDuplicatePlaceholderPartitionPath() {
+        ValidationException e = expectThrows(
+            ValidationException.class,
+            () -> validator.validateDataset(Map.of(), "s3://b/p", Map.of("partition_path", "{year}/junk/{year}"))
+        );
+        assertThat(e.getMessage(), containsString("partition_path"));
+        assertThat(e.getMessage(), containsString("more than once"));
+    }
+
     public void testValidateDatasetHivePartitioning() {
         assertEquals(false, validator.validateDataset(Map.of(), "s3://b/p", Map.of("hive_partitioning", false)).get("hive_partitioning"));
         assertEquals(true, validator.validateDataset(Map.of(), "s3://b/p", Map.of("hive_partitioning", true)).get("hive_partitioning"));

x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/ExternalCsvHivePartitionedIT.java

@@ -79,29 +79,21 @@ public void testHivePartitioningValidatesAndParses() throws Exception {
     }
 
     /**
-     * Same fixture, with an explicit {@code partition_path}. This template names NO columns — the detector matches a
-     * path segment in full, so {@code year={year}} is not a placeholder. With no {@code partition_detection} the
-     * strategy is AUTO, which tries Hive first, so the dataset keeps the {@code year}/{@code month} columns it had
-     * before the setting reached the read path.
-     *
-     * <p>It previously asserted only that the query did not fail with "unknown option [partition_path]". An
-     * acceptance assertion cannot tell an applied template from an unapplied one, so it passed while the setting
-     * was unread. It now asserts the resulting columns.
+     * Same Hive-shaped fixture with a legal {@code partition_path}. Strategy is AUTO, so Hive runs first and
+     * the query still sees {@code year}, {@code month}, and the physical {@code id} column.
      */
     public void testPartitionPathValidatesAndParses() throws Exception {
         Path root = createTempDir().resolve("template_csv");
         writePartitionedCsvFiles(root);
 
         @SuppressWarnings("checkstyle:EmptyJavadoc") // checkstyle thinks this is Javadoc
         String glob = StoragePath.fileUri(root) + "/**/*.csv";
-        String dataset = registerDataset("template_csv", glob, Map.of("partition_path", "year={year}/month={month}/*.csv"));
+        String dataset = registerDataset("template_csv", glob, Map.of("partition_path", "{year}/{month}"));
         String query = "FROM " + dataset + " | LIMIT 1";
 
         try (var response = run(syncEsqlQueryRequest(query))) {
             List<String> columnNames = response.columns().stream().map(c -> c.name()).collect(Collectors.toList());
             assertThat("the data columns must still be read", columnNames, hasItem("id"));
-            // The template names no columns, so AUTO falls through to Hive detection -- the same columns this
-            // dataset produced before partition_path reached the read path.
             assertThat("the Hive-derived year must still appear", columnNames, hasItem("year"));
             assertThat("and the Hive-derived month", columnNames, hasItem("month"));
         }

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/datasources/PartitionConfig.java

@@ -8,7 +8,11 @@
 package org.elasticsearch.xpack.esql.datasources;
 
 import org.elasticsearch.core.Nullable;
+import org.elasticsearch.xpack.esql.datasources.TemplatePartitionDetector.TemplateSegment;
+import org.elasticsearch.xpack.esql.datasources.spi.StoragePath;
 
+import java.util.HashSet;
+import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
@@ -196,5 +200,80 @@ public static void validate(Map<String, Object> config) {
                     + "] or enable partition detection"
             );
         }
+
+        if (hasTemplate) {
+            validatePathTemplate(template);
+        }
+    }
+
+    /**
+     * A {@code partition_path} must name each column once as a whole-segment {@code {name}}, and every other
+     * segment must be a concrete directory name. A glob-shaped literal cannot match a real directory. A
+     * repeated placeholder cannot be one column value when the two slots differ.
+     */
+    private static void validatePathTemplate(String template) {
+        List<TemplateSegment> parsed = TemplatePartitionDetector.parseTemplate(template);
+        Set<String> names = new HashSet<>();
+        boolean hasPlaceholder = false;
+        for (TemplateSegment segment : parsed) {
+            if (segment instanceof TemplateSegment.Placeholder(String name)) {
+                hasPlaceholder = true;
+                if (names.add(name) == false) {
+                    throw new IllegalArgumentException(
+                        "["
+                            + CONFIG_PARTITIONING_PATH
+                            + "] ["
+                            + template
+                            + "] names ["
+                            + name
+                            + "] more than once; each partition column must appear once"
+                    );
+                }
+            }
+        }
+        if (hasPlaceholder == false) {
+            throw new IllegalArgumentException(
+                "["
+                    + CONFIG_PARTITIONING_PATH
+                    + "] ["
+                    + template
+                    + "] names no columns; a partition column must be a path segment that is exactly {name}, such as "
+                    + "[{year}/{month}]. For a key=value directory layout set ["
+                    + CONFIG_PARTITIONING_DETECTION
+                    + "] to [hive] instead of a template"
+            );
+        }
+        // year={year} also contains '{', so this scan runs after the empty-column throw: that
+        // template must report that it names no columns, not that a segment is glob-shaped.
+        for (TemplateSegment segment : parsed) {
+            if (segment instanceof TemplateSegment.Literal(String value)) {
+                if (TemplatePartitionDetector.containsEmbeddedPlaceholder(value)) {
+                    throw new IllegalArgumentException(
+                        "["
+                            + CONFIG_PARTITIONING_PATH
+                            + "] ["
+                            + template
+                            + "] has a key=value segment ["
+                            + value
+                            + "]; a partition column must be a path segment that is exactly {name}, such as "
+                            + "[{year}/{month}]. For a key=value directory layout set ["
+                            + CONFIG_PARTITIONING_DETECTION
+                            + "] to [hive] instead of a template"
+                    );
+                }
+                if (StoragePath.containsGlobMetacharacter(value)) {
+                    throw new IllegalArgumentException(
+                        "["
+                            + CONFIG_PARTITIONING_PATH
+                            + "] ["
+                            + template
+                            + "] has a glob-shaped segment ["
+                            + value
+                            + "]; a non-placeholder segment is a required directory name, such as "
+                            + "[junk] in [{year}/junk/{month}]"
+                    );
+                }
+            }
+        }
     }
 }