Added semantic check for calls to s.bag() from a UDF of s.updateWith* #85
Conversation
ggevay
force-pushed
the
semantic-check-stateful-udf-bag-access
branch
3 times, most recently
from
5376edc
to
9d0dbda
Compare
|
I did some minor modifications on the PR:
|
ggevay
force-pushed
the
semantic-check-stateful-udf-bag-access
branch
2 times, most recently
from
48866f2
to
b257cd5
Compare
|
|
||
| class InvalidProgramException(message: String = null, cause: Throwable = null) extends Exception(message, cause) {} | ||
|
|
||
| class StatefulAccessedFromUdfException() |
There was a problem hiding this comment.
Exceptions can be defined in the api package.
There was a problem hiding this comment.
OK, I have moved it there.
| @@ -241,16 +242,19 @@ private[emma] trait ComprehensionAnalysis | |||
| // stateful.updateWithZero(udf) | |||
| case q"${updateWithZero @ Select(ident @ Ident(_), _)}[$_]($udf)" | |||
| if updateWithZero.symbol == api.updateWithZero => | |||
| CheckForStatefulAccessedFromUdf(ident, udf) | |||
There was a problem hiding this comment.
I think that the semantics checks should be implemented as a separate set of traversers chained together via the ->> combinator (similarly to the normalization chain).
It will also allow us to keep things clearly separated in the long run.
There was a problem hiding this comment.
That would have the drawback that some of the functionality of comprehend would be duplicated. I mean, there would be a similar match with the same cases. Do you think this is worth it?
There was a problem hiding this comment.
Agreed, there will be some duplication, but on the other hand we cleaner code and separation of concerns.
There was a problem hiding this comment.
I would prefer if the checks are added as separate functions and chain together in a similar way as the normalize* functions (can happen before the normalization phase).
There was a problem hiding this comment.
OK, I will do it that way then.
ggevay
force-pushed
the
semantic-check-stateful-udf-bag-access
branch
4 times, most recently
from
6eb633b
to
8768204
Compare
|
Thanks, I'll merge this in the next batch. |
| s <- DataBag(state.get(k(a)).toList) | ||
| b <- f(s, a) | ||
| } yield b | ||
| (k: A => K, f: (S, A) => DataBag[B]): DataBag[B] = { |
There was a problem hiding this comment.
The outer block is redundant.
|
I've added two minor remarks regarding code formatting and naming. Otherwise I think this can be merged after #88. |
ggevay
force-pushed
the
semantic-check-stateful-udf-bag-access
branch
from
8768204
to
436a5da
Compare
ggevay
force-pushed
the
semantic-check-stateful-udf-bag-access
branch
from
436a5da
to
26e3157
Compare
|
Thanks, I have addressed all the above comments, and rebased to the current master. I will merge this shortly. |
This is solving the first issue in #84. I implemented what I wrote in the comment there.
(This PR is on top of the commit of #83. I will rebase this, after that is merged.)