The plan becomes not so easier to read, which is more important to me. 
Unless we change $ to other symbol, I am inclined to keep using Java.

I am not sure why schemas in MaterializationTest require column names to be 
quoted, If we can change that, queries will be much easier to read (even for 
multiline strings), like RelOptRulesTest does.

- Haisheng

------------------------------------------------------------------
发件人:Vladimir Sitnikov<sitnikov.vladi...@gmail.com>
日 期:2019年12月15日 06:09:37
收件人:Apache Calcite dev list<dev@calcite.apache.org>
主 题:[DISCUSS] Tests vs multiline strings

Hi,

Calcite tests sometimes require multiline strings.
For instance: input SQL, output plan.

TL;DR: let's use Kotlin to improve test code readability. What do you think?

Unfortunately, Java <14 lacks multiline strings, so the current tests are
not that pretty :(
They are hard to read, hard to understand, and it is hard to maintain them.

Sample test (from MaterializationTest):

  @Test public void testFilterQueryOnProjectView5() {
    checkMaterialize(
        "select \"deptno\" - 10 as \"x\", \"empid\" + 1 as ee, \"name\"\n"
            + "from \"emps\"",
        "select \"name\", \"empid\" + 1 as e\n"
            + "from \"emps\" where \"deptno\" - 10 = 2",
        HR_FKUK_MODEL,
        CalciteAssert.checkResultContains(
            "EnumerableCalc(expr#0..2=[{inputs}], expr#3=[2], "
                + "expr#4=[=($t0, $t3)], name=[$t2], E=[$t1],
$condition=[$t4])\n"
                + "  EnumerableTableScan(table=[[hr, m0]]"));
  }

What do you think if we migrate those types of tests to Kotlin?
Note: I'm well aware there are Quidem-based tests, and those do not replace
Java-based ones.

Here's how the same test might look like in Kotlin:

    @Test
    fun `filter query on project view5`() {
        checkMaterialize(
            """select "deptno" - 10 as "x", "empid" + 1 as ee, "name" from
"emps" """,
            """select "name", "empid" + 1 as e from "emps" where "deptno" -
10 = 2""",
            HR_FKUK_MODEL,
            CalciteAssert.checkResultContains(
                """
                EnumerableCalc(expr#0..2=[{inputs}], expr#3=[2],
expr#4=[=(${'$'}t0, ${'$'}t3)], name=[${'$'}t2], E=[${'$'}t1],
${'$'}condition=[${'$'}t4])
                  EnumerableTableScan(table=[[hr, m0]]
                """.trimIndent()
            )
        )
    }

Pros:
* SQLs are much easier to read since all the slashes are gone
* Multiline text is much easier to follow
* ctrl+c/v works much better: I can copy a substring, even multiple lines
from the middle, and it just works
* PR review would be simplified as SQL in PRs would be easier to read

Cons:
* A bit of a sad point is $ is a special symbol there, and it needs to be
escaped.
A (not that?) crazy idea might be to make Calcite use another character
instead of $ (as $ is used in string templating languages quite often
anyway),
* If a class is converted from Java to Kotlin, then default Git annotate
fails to attribute lines to commits (== the file looks like a brand new one)
* Something else?

I don't suggest to go ahead and convert all the things, however, in my
opinion, converting at least some of the tests or using multiline strings
for newly added tests makes sense.

What do you think?

Vladimir

Reply via email to