Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open peer review for NMF solution #4

Open
agarciadom opened this issue Jun 10, 2021 · 0 comments
Open

Open peer review for NMF solution #4

agarciadom opened this issue Jun 10, 2021 · 0 comments
Labels
question Further information is requested

Comments

@agarciadom
Copy link
Member

agarciadom commented Jun 10, 2021

I had no issues running the solution: I installed .NET 3.1 through the usual Microsoft instructions for Linux, then changed the config/config.json file to use the NMF solution, and then I ran the solution through scripts/run.py -bm while the Docker MySQL image was running. I think it would still be good to explicitly mention the version of .NET needed in the README, as a subsection of "Solution prerequisites".

The solution ran fast, and produced small and fast queries which passed all correctness checks. Rather than read the original .res files, the solution mapped their contents to .NET code and directly compared against that.

I did not know about the dynamic dispatch in .NET, but it looks really nice: much better than having to implement a Visitor pattern manually, and most likely just as fast or even faster. As a Java developer, I'm a little envious :-).

One thing I noticed was that rather than implementing directly the mapping at the end of the OCL2PSQL case paper, this solution uses the concept of a mutable "select content" where the joins, FROM clause and select items are changed on the go, while traversing recursively over the solution. While this seems to work just fine (judging from the passing of the correctness checks, and by the generated queries which do appear to map well the intent of the original OCL queries), in my opinion it makes it a bit more difficult to judge whether the original OCL2PSQL mapping had been implemented, and whether these shortcuts may result in later issues if the rest of the mapping were to be implemented. In summary, there is a OCL to pure SQL mapping here indeed, and it's working well for the given scenarios, but it may not entirely be that OCL2PSQL mapping in the original paper.

For instance, I notice that the original val columns don't seem to appear much if at all in this solution. It'd be interesting to have a conversation with the authors of the original OCL2PSQL mapping on its real necessity

Great job on the join pruning! I may steal the idea given more time - I think there could be scope in the Epsilon solution for adding a number of "rewrite rules" to simplify the SQL query after the initial mapping. I do think that a more "direct" mapping at first followed by optimizing rewrite rules could be easier to maintain in the longer run - this may be worth discussing at the TTC :-).

Two minor corrections for the paper:

  • "knows to collections" -> "knows two collections"
  • "thus the result get incorrect" -> "thus the result becomes incorrect"

/cc @georghinkel

@agarciadom agarciadom added the question Further information is requested label Jun 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

1 participant