IGNITE-28596 Add test to check TPCH plans#13068
Conversation
|
| TpchHelper.createTables(srv); | ||
|
|
||
| /* | ||
| TpchHelper.fillTables(srv, 0.01); |
There was a problem hiding this comment.
seems it need to be fixed somehow ?
| * @return {@code True} if any expanded plan equals to the parameter. | ||
| */ | ||
| public boolean match(String actualPlan) { | ||
| return expandOneof(template).stream() |
There was a problem hiding this comment.
| return expandOneof(template).stream() | |
| return expandOneof(template).stream() |
|
Overall looks good instead i still believe that non-empty tables size need to be involved, probably places like : |
| /** | ||
| * Abstract test class to ensure a planner generates expected plan for TPC queries. | ||
| */ | ||
| public class AbstractTpcQueryPlannerTest extends AbstractPlannerTest { |
There was a problem hiding this comment.
It's not a planner test, it's an integration test, you almost not use AbstractPlannerTest except plannerCtx, but you don't need it, the same result can be obtained by just:
sql("EXPLAIN PLAN FOR " + loadFromResource(sqlTestName(getClass()) + "/" + queryId + ".sql"))
So, it's more correct to inherit from AbstractBasicIntegrationTest
|
|
||
| /** Query id. Set by {@link PlanChecker}. */ | ||
| @Parameterized.Parameter | ||
| public String queryId; |
There was a problem hiding this comment.
Abbreviation should be used
| /** Run once, before all queries check. */ | ||
| @BeforePlansTest | ||
| public static void startAll(Class<?> testClass) throws Exception { | ||
| AbstractTpcQueryPlannerTest mock = new AbstractTpcQueryPlannerTest(); | ||
|
|
||
| mock.beforeFirstTest(); | ||
|
|
||
| IgniteConfiguration cfg = mock.getConfiguration("server"); | ||
|
|
||
| cfg.getSqlConfiguration() | ||
| .setQueryEnginesConfiguration(new CalciteQueryEngineConfiguration().setDefault(true)); | ||
|
|
||
| srv = (IgniteEx)IgnitionEx.start(cfg); | ||
|
|
||
| srv.getOrCreateCache(new CacheConfiguration<>("mock") | ||
| .setSqlFunctionClasses(AbstractTpcQueryPlannerTest.TpchUDF.class) | ||
| .setSqlSchema("PUBLIC")); | ||
|
|
||
| TpchHelper.createTables(srv); | ||
|
|
||
| /* | ||
| TpchHelper.fillTables(srv, 0.01); | ||
|
|
||
| TpchHelper.collectSqlStatistics(srv); | ||
| */ | ||
| } | ||
|
|
||
| /** Run once, after all queries check. */ | ||
| @AfterPlansTest | ||
| public static void stopAll(Class<?> testClass) { | ||
| if (srv != null) { | ||
| srv.close(); | ||
|
|
||
| srv = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Overcomplicated, it's hard to read and understand. All PlanChecker related code (together with PlanChecker itself) can be replaced with standard parameterized test, all you need is one method:
@Parameterized.Parameters(name = "queryId={0}")
public static Collection<String> params() throws IOException {
return Files.list(Path.of(RSRC_DIR, "tpch"))
.filter(p -> p.toString().endsWith(".sql") && !p.toString().endsWith("ddl.sql"))
.sorted()
.map(p -> p.getFileName().toString().replace(".sql", ""))
.collect(Collectors.toList());
}
Or extend AbstractTpchTest and redefine set of tests.
| * @param sql SQL query. | ||
| * @param params Query parameters. | ||
| */ | ||
| public static List<List<?>> sql(Ignite ignite, String sql, Object... params) { |
There was a problem hiding this comment.
Not needed if test will be inherited from AbstractBasicIntegrationTest
| } | ||
|
|
||
| /** */ | ||
| public static class TpchUDF { |
There was a problem hiding this comment.
UDF not used at all
| */ | ||
| public class PlanTemplate { | ||
| /** */ | ||
| private static final Pattern ID_PATTERN = Pattern.compile(", id = \\d+"); |
There was a problem hiding this comment.
Maybe it's better to remove "id" from plan. It's not needed for user at all (only required for developer, for planner dumps).
| private static final Pattern ID_PATTERN = Pattern.compile(", id = \\d+"); | ||
|
|
||
| /** */ | ||
| private static final Pattern HASH_PATTERN = Pattern.compile(", hash=-?\\d+]"); |
There was a problem hiding this comment.
Maybe it's better to beautify AffinityIdentity.toString() (add annotation @GridToStringExclude to hash field)?
| while (sc.hasNextLine()) { | ||
| String line = sc.nextLine(); | ||
|
|
||
| for (StringBuffer expanded : res) { |
| } | ||
|
|
||
| /** Expands plans. Replace {ONEOF} tag with the possible cases. */ | ||
| private static List<String> expandOneof(String plan) { |
There was a problem hiding this comment.
Oneof -> OneOf
Inconsistent API with expandIndexCase (one returns Stream, another returns List)
| TpchHelper.createTables(srv); | ||
|
|
||
| /* | ||
| TpchHelper.fillTables(srv, 0.01); |
There was a problem hiding this comment.
It takes about 5 seconds to fill with 0.01 scale, but in this case plans are more predictable. Plans without data are irrelevant, I this we should fill the tables.
| } | ||
|
|
||
| /** Expands plans. Replace {INDEX_CASE} tag with the possible indexes. */ | ||
| private static Stream<String> expandIndexCase(String plan) { |
There was a problem hiding this comment.
All these cases with {INDEX_CASE} are related to proxy indexes. Maybe just replace index=[{index_name}_proxy] with index=[{index_name}] and don't use {INDEX_CASE} at all?
3 participants
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see tabPR Checkat TC.Bot - Instance 1 or TC.Bot - Instance 2)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.