Parser needs to differentiate between bags and record constructors

[GoogleCodeExporter]

The following AQL statement produces a syntax error because the foo and baz 
record constructors both end with "}" and are next to each other. The parser 
treats this as the close of a bag constructor when really it should be able to 
differentiate between end of record constructor and end of bag constructor. 
This can be achieved by making the parser stateful: it should know when it's 
"in" a record constructor (and should treat }} as two separate lexical 
tokens)... similarly for bags.

I think Till his this issue before as well and my guess is that he solved it by 
spacing out the braces (ouch!). It's pretty painful to have to do this and 
probably also error prone if you are programmatically manipulating an AQL 
string and it actually contained a bag constructor.

Till, I'm assigning this to you for now, but feel free to delegate.
-----------------------------------------
drop dataverse barverse if exists;
create dataverse barverse;
use dataverse barverse;

create type bartype as open {
"id": int32
}

create dataset barset(bartype) primary key id;

insert into dataset barset (
{ 
    "id": 1,
    "foo": {
        "crudge": 1,
        "baz": {
    }}
}
);
---------------------------------

Original issue reported on code.google.com by zheilb...@gmail.com on 31 Jul 2013 at 10:04

[GoogleCodeExporter]

Original comment by zheilb...@gmail.com on 31 Jul 2013 at 10:22

• Added labels: Severity-MSRBlocker

[GoogleCodeExporter]

Well we do have issue 429 for this, but we decided to document it instead of 
fixing it for beta.
But I agree that we should fix it now.

Original comment by westm...@gmail.com on 31 Jul 2013 at 11:50

[GoogleCodeExporter]

Issue 429 has been merged into this issue.

Original comment by zheilb...@gmail.com on 1 Aug 2013 at 12:49

[GoogleCodeExporter]

I've got a fix in westmann/parser_fixes that works for this query. 
Unfortunately, it introduces a test failures that I cannot reproduce in 
isolation (it happens when I run the full suite but not when I run the 
individual test).

Original comment by westm...@gmail.com on 1 Aug 2013 at 9:40

[GoogleCodeExporter]

Ah, the joy of stateful things...  :-)

Original comment by dtab...@gmail.com on 1 Aug 2013 at 4:16

[GoogleCodeExporter]

Ah, okay, strange that the state persists across test cases... 
Let me know when you are ready for a code review and I will do it.

Original comment by zheilb...@gmail.com on 1 Aug 2013 at 7:57

[GoogleCodeExporter]

Original comment by zheilb...@gmail.com on 1 Aug 2013 at 7:57

• Added labels: Severity-MSRImpediment
• Removed labels: Severity-MSRBlocker

[GoogleCodeExporter]

Theory developed from Till saying Yingyi's name and showing me the query:  
Yingyi recently changed the sorting (used in dup-elim too) to randomize its 
input frames to avoid evil corner case behavior due to input data ordering.  If 
he truly randomizes, i.e., using rand(), then this will be non-deterministic, 
potentially, depending on the seed that the test case happens to get as its 
starting state.

Original comment by dtab...@gmail.com on 2 Aug 2013 at 2:40

[GoogleCodeExporter]

Ok, I've changed the test such that it produces a deterministic result.
I think we're ready for a review (and if there's a a better/more compact way to 
do this I'm happy to change the implementation).

Original comment by westm...@gmail.com on 2 Aug 2013 at 6:44

[GoogleCodeExporter]

Okay, I'll have a look. Send the review for both over whenever you're ready.

Original comment by zheilb...@gmail.com on 2 Aug 2013 at 7:06

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Here's the link to the Rietveld review of the complete change: 
https://codereview.appspot.com/12328043

Thanks!

Original comment by westm...@gmail.com on 2 Aug 2013 at 7:22

[GoogleCodeExporter]

rev 21cb90656304

Original comment by westm...@gmail.com on 2 Aug 2013 at 8:20

• Changed state: Fixed