feat: distinguish plan generation from validation in functional tests

This PR adds a new test target for the `relation` and
`extension_function` producer tests such that now both have one variant
that validates the plans whereas the other one doesn't. Validation is
useful to see how conformant the producer is. Not activating it is
useful to allow non-conformant plans to be generated, which can then be
used as input for consumer tests (although it is debatable whether
that's a good idea but that's a discussion for another day). The TPC-H
tests already do this distinction, and replicating that is the last step
before the entry scripts of all categories can be merged.

Signed-off-by: Ingo Müller <[email protected]>

substrait_consumer/tests/functional/extension_functions/test_sql_producers.py

@@ -12,7 +12,7 @@
 TEST_CASE_PATHS = list(
     (path.relative_to(CONFIG_DIR),) for path in FUNCTION_CONFIG_DIR.rglob("*.json")
 )
-IDS = (str(path[0]).removesuffix(".json") for path in TEST_CASE_PATHS)
+IDS = list(str(path[0]).removesuffix(".json") for path in TEST_CASE_PATHS)
 
 
 @pytest.mark.parametrize(["path"], TEST_CASE_PATHS, ids=IDS)
@@ -41,3 +41,32 @@ def test_sql_producer(
         producer,
         validate=False,
     )
+
+
+@pytest.mark.parametrize(["path"], TEST_CASE_PATHS, ids=IDS)
+@pytest.mark.usefixtures("prepare_tpch_parquet_data")
+@pytest.mark.usefixtures("prepare_small_tpch_parquet_data")
+def test_sql_producer_valid(
+    path: Path,
+    snapshot: Snapshot,
+    record_property,
+    db_con: duckdb.DuckDBPyConnection,
+    producer,
+):
+    test_case = load_json(CONFIG_DIR / path)
+    local_files = test_case["local_files"]
+    named_tables = test_case["named_tables"]
+    sql_query = test_case["sql_query"]
+    # Workaround to distinguish validity test from generation test:
+    path = Path(str(path).removesuffix(".json") + "-validate")
+    substrait_producer_sql_test(
+        path,
+        snapshot,
+        record_property,
+        db_con,
+        local_files,
+        named_tables,
+        sql_query,
+        producer,
+        validate=True,
+    )

substrait_consumer/tests/functional/relations/test_sql_producers.py

@@ -12,12 +12,23 @@
 TEST_CASE_PATHS = list(
     (path.relative_to(CONFIG_DIR),) for path in RELATION_CONFIG_DIR.rglob("*.json")
 )
-IDS = (str(path[0]).removesuffix(".json") for path in TEST_CASE_PATHS)
+IDS = list(str(path[0]).removesuffix(".json") for path in TEST_CASE_PATHS)
+
+
+@pytest.fixture
+def mark_producer_tests_as_xfail(request):
+    """Marks a subset of tests as expected to be fail."""
+    path = request.getfixturevalue("path")
+    if "relation/read/duckdb_read_local_file" in str(path):
+        pytest.skip(
+            reason="'duckdb_read_local_file' contains an absolute path, which we can't deal with yet"
+        )
 
 
 @pytest.mark.parametrize(["path"], TEST_CASE_PATHS, ids=IDS)
 @pytest.mark.usefixtures("prepare_tpch_parquet_data")
 @pytest.mark.usefixtures("prepare_small_tpch_parquet_data")
+@pytest.mark.usefixtures("mark_producer_tests_as_xfail")
 @pytest.mark.produce_substrait_snapshot
 def test_sql_producer(
     path: Path,
@@ -30,6 +41,35 @@ def test_sql_producer(
     local_files = test_case["local_files"]
     named_tables = test_case["named_tables"]
     sql_query = test_case["sql_query"]
+    substrait_producer_sql_test(
+        path,
+        snapshot,
+        record_property,
+        db_con,
+        local_files,
+        named_tables,
+        sql_query,
+        producer,
+        validate=False,
+    )
+
+
+@pytest.mark.parametrize(["path"], TEST_CASE_PATHS, ids=IDS)
+@pytest.mark.usefixtures("prepare_tpch_parquet_data")
+@pytest.mark.usefixtures("prepare_small_tpch_parquet_data")
+def test_sql_producer_valid(
+    path: Path,
+    snapshot: Snapshot,
+    record_property,
+    db_con: duckdb.DuckDBPyConnection,
+    producer,
+):
+    test_case = load_json(CONFIG_DIR / path)
+    local_files = test_case["local_files"]
+    named_tables = test_case["named_tables"]
+    sql_query = test_case["sql_query"]
+    # Workaround to distinguish validity test from generation test:
+    path = Path(str(path).removesuffix(".json") + "-validate")
     substrait_producer_sql_test(
         path,
         snapshot,