You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@drill.apache.org by paul-rogers <gi...@git.apache.org> on 2016/12/08 05:48:59 UTC

[GitHub] drill pull request #684: DRILL-5070: Code gen: create methods in fixed order...

GitHub user paul-rogers opened a pull request:

    https://github.com/apache/drill/pull/684

    DRILL-5070: Code gen: create methods in fixed order to allow test verification	

    A handy technique in testing is to compare generated code against a "golden" copy that defines the expected results. However, at present, Drill generates code using the method order returned by Class.getDeclaredMethods, but this method makes no guarantee about the order of the methods. The order varies from one run to the next. There is some evidence this link that order can vary even within a single run, though a quick test was unable to reproduce this case.
    
    The fix is simple, in the SignatureHolder constructor, sort methods by name after retrieving them from the class. The sort ensures that method order is deterministic. Fortunately, the number of methods is small, so the sort step adds little cost.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/paul-rogers/drill DRILL-5070

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/drill/pull/684.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #684
    
----
commit 63d9b5aaffb6982355c785efe26dfede81c0a66a
Author: Paul Rogers <pr...@maprtech.com>
Date:   2016-11-28T05:51:02Z

    DRILL-5070: Actual fix
    
    The fix: order methods by name in the \u201csignature\u201d so that generated
    methods occur in the same order every time.

commit b25bbf5c25c23c196710af0e72b79cc97acff0e9
Author: Paul Rogers <pr...@maprtech.com>
Date:   2016-11-28T05:52:41Z

    DRILL-5070: Test cases
    
    Modifies the existing ExpressionTest to capture generated code and
    compare it to a \u201cgolden\u201d copy. Provides the mechanism to do the
    comparison.
    
    Before the fix, comparisons randomly fail due to random method order.
    After the fix, the comparisons consistently work, showing that code is
    generated in the same order each time.

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill issue #684: DRILL-5070: Code gen: create methods in fixed order to all...

Posted by jinfengni <gi...@git.apache.org>.

Github user jinfengni commented on the issue:

    https://github.com/apache/drill/pull/684
  
    It's fine to have fixed order for the generated methods. I'm not sure if it's appropriate to enforce  "golden" copy in the new tests. The golden copy is just one implementation in the current code; people may change the implementation in the future, and it's likely these new testcases will fail. The person to make the change in the future will have to deal with the overhead.
    
    The question is what benefit we get from enforcing such "golden" copy in the testcases.  


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] drill issue #684: DRILL-5070: Code gen: create methods in fixed order to all...

Posted by paul-rogers <gi...@git.apache.org>.

Github user paul-rogers commented on the issue:

https://github.com/apache/drill/pull/684

Golden copies are needed only for tests that, today, do their "testing" by dumping generated code to stdout. Such tests do absolutely nothing (other than detect crashes) when run under Maven. The golden copy assures that such low-level tests are verified.

Other tests verify results by executing code. No need to save a golden copy of code for those.

Yes, when code gen changes, the golden copies have to be recaptured. However, how else can we verify that the new changes do what we expect if we don't actually examine them and save the desired final state? How else do we detect unexpected changes to generated code? (For example, if we generated a bunch of unneeded boilerplate, we'd not detect that in an execution test.)

This test is also going to be a way to verify DRILL-5052: capture code using the current code gen technique, then verify against the code path described in DRILL-5052. The two paths should be identical except for an extra "extends" in the code from DRILL-5052.

Is it worth testing such cases or is it OK to simply trust that the generated code is probably OK?

---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---