search

mit-pdos / noria

Fast web applications through dynamic, partially-stateful dataflow
Apache License 2.0
4.99k stars 244 forks source link

Merge security logic #60

Closed larat7 closed 6 years ago

larat7 commented 6 years ago

Merge the security-merge branch into master to avoid having to keep track of both of them. Currently it passes the same benchmarks and tests as master.

jonhoo commented 6 years ago

It looks like at least one test fails on Travis: https://travis-ci.org/mit-pdos/distributary/builds/347553194#L949

ms705 commented 6 years ago

Travis runs cargo test --all, and hence caught this.

From a quick look, the change originates in c2ed208ca, which adds code for creating group context nodes. I suspect the generation of the group context node name converts a DataType to string, but isn't supposed to quote it because that string get concatenated with others to form the name.

larat7 commented 6 years ago

Yeah, I often need to format node names for specific universes and treating DataType as an unquoted string makes it way more convenient. Any suggestions?

Edit: @ms705 talked and I will just add a to_string method for DataType

ms705 commented 6 years ago

This looks great! I left some comments for us to chase, but from what I can tell, there are no fundamental obstacles to merging. The main correctness question lingers around the changed semantics for matching columns in many places (by column name only, not by table name now).

This PR actually also splits the behemoth file in src/controller/sql/mir.rs into more manageable submodules, a nice side effect.

jonhoo commented 6 years ago

This is looking pretty good! I've done a pass over the more dataflow-y parts of the code, and left some comments. It'd probably be good to have @fintelia look over the controller changes, though nothing particularly egregious stood out to me there.

larat7 commented 6 years ago

I think we might be ready to merge this. @jonhoo @ms705 @fintelia ?

ms705 commented 6 years ago

:+1: Go ahead!