Check $ use in variables and coverage defs

[abeccati]

Both following queries shall work:

This fails:
for $c in ( NIR )
return

encode( $c [ x(0:450), y(0:300) ], "png" )

This works:
for $c in ( NIR )
return

encode( $c, "png" )

[abeccati]

WCPS standard allows both with and without $, to be verified and provide consistent behavior

[abeccati]

Queries have been tested on EarthLook

[Piero Campalani]

Working on the solution, almost there.

[Piero Campalani]

I see no (relatively straightforward) way to implement this feature: I successfully ran the proposed queries but there is a conflict between coverage-variable and variable names at parsing stage (petascope.wcps.server.grammar).

One way would be — after parsing — to collect the coverage variable names as shared static set in the common AbtractRasNode.java class, so that when a NumericScalarExpr where:

   nodeName.equals(WCPSConstants.MSG_VARIABLE_REF)

one could check if the variable is a coverage label or a simple variable.

This would anyway mean that sometimes a parsed variableRef node in the query would be treated as a coverage node.
It is a feasible but still ugly way to work it out, imho.

Are there further ideas?
Alan, I would suggest to propose a change in the WCPS instead, and close this as wontfix. Or either let it open until some other dev wants to

Detailed explanation : if we enable '$' prefixes in the coverage-variable names, like this (petascope.wcps.grammar.wcps.g):

coverageVariable returns[String value]
---	: var=NAME { $value = $var.text; }
+++	: var=(NAME | VARIABLE_DOLLAR) { $value = $var.text; }
	;

The $c names in the query are correctly identified as iterator variables and indeed the parser directly looks for a 'coverageVariable':

forClause returns[ForClauseElements value]
	: FOR v=coverageVariable IN LPAREN list=coverageList RPAREN
...

but this cannot happen inside the 'returnClause' where the 'scalarExpr' needs to be tried before a 'coverageVariable' otherwise the WCPS engine breaks up:

coverageAtomConstructor returns[CoverageExpr value]
    : e1=subsetExpr  { $value = new CoverageExpr($e1.value); }
    | e2=unaryInducedExpr { $value = $e2.value; }
    | e3=scaleExpr { $value = new CoverageExpr($e3.value); }
    | e4=crsTransformExpr { $value = new CoverageExpr($e4.value); }
 *  | e5=coverageAtom { $value = $e5.value; }
...
coverageAtom returns[CoverageExpr value]
    : e1=scalarExpr { $value = new CoverageExpr($e1.value); }
    | e2=coverageVariable { $value = new CoverageExpr($e2.value); }
...

Parsing a 'scalarExpr' beforehand indeed means that successfully an element 'numericScalarExpr'->'variableName' is parsed instead of a 'coverageVariable':

numericScalarExpr returns[NumericScalarExpr value]
	: e1=numericScalarTerm {$value = $e1.value; }
	  (op=(PLUS|MINUS) e2=numericScalarTerm { $value = new NumericScalarExpr($op.text, $value, $e2.value); })*
	;
numericScalarTerm returns[NumericScalarExpr value]
	: e1=numericScalarFactor { $value = $e1.value; }
		(op=(MULT|DIVIDE) e2=numericScalarFactor { $value = new NumericScalarExpr($op.text, $value, $e2.value); })*
	;
numericScalarFactor returns[NumericScalarExpr value]
    : LPAREN e1=numericScalarExpr RPAREN { $value = $e1.value; }
    | op=MINUS e10=numericScalarFactor { $value = new NumericScalarExpr($op.text, $e10.value); }
    | op=ABS LPAREN e12=numericScalarExpr RPAREN { $value = new NumericScalarExpr($op.text, $e12.value); }
    | op=SQRT LPAREN e11=numericScalarExpr RPAREN { $value = new NumericScalarExpr($op.text, $e11.value); }
    | op=ROUND LPAREN e1=numericScalarExpr RPAREN { $value = new NumericScalarExpr($op.text, $e1.value); }
    | e=INTEGERCONSTANT { $value = new NumericScalarExpr($e.text); }
    | e=FLOATCONSTANT { $value = new NumericScalarExpr($e.text); }
    | e2=complexConstant { $value = new NumericScalarExpr($e2.value); }
    | e3=condenseExpr { $value = new NumericScalarExpr($e3.value); }
    | e4=variableName { $value = new NumericScalarExpr("var", $e4.value); } // -> CONFLICT!

[Piero Campalani]

Alan,
this query you posted:

for $c in ( NIR )
return encode( $c, "png" )

works just because $c has no further branches below it as a node (no trims, slices, etc.). It is wrongly parsed as variableRef (and not coverage as it should), but no further analysis on the node are required so the inconsistency does not arise, and in the end the label is set to null since there is no variable definition for $c :

TRACE [14:50:22] VariableReference@46:   variable c has been renamed into null

and the final RasQL query is:

select encode(null, "PNG") from NIR AS null

which works, what, let's say, just a chance.

[Piero Campalani]

A working solution would be to unify the variable names for both coverages (for % in ..) and axes (over % x (0:255) ...) onto a single type variableName, as by WCPS standard. The grammar would define its syntax as:

VARIABLE_NAME: ('$'|'a'..'z'|'A'..'Z'|'_')(('a'..'z'|'A'..'Z'|'0'..'9'|'_')*);

Afterwards, in the wcps.server.core classes, a dictionary of variable names and associated type (coverage or axis) should be built: then one would be able to differentiate.

The classes involved are:

petascope.wcps.server.core.
  + CoverageIterator.java       // cov var
  + ConstructCoverageExpr.java  // axis var
  + ConstantCoverageExpr.java   // axis var
  + CondenseScalarExpr.java     // axis var

[abeccati]

I see,
the problem then lies in the use of different elements to distinguish variable types, which avoided a look-up table. The solution look good to me (defined variables table for type checking), but look work for a future milestone.

As of 8.4 we have this exception wrt WCPS then:

VariableNames used to hold coverages (i.e. in a "for" clause) MUST NOT start with a dollar sign.
VarableNames used to identify axes (i.e. in an "over" clause) MUST start with a dollar sign

hence this is the correct syntax

for c in ( mr )
return  encode(
          coverage histogram
          over $n x (0:2)
          values count( c = $n )
      , "csv" )

So that in the values clause variable types are distinguishable by $.
This should be also noted in the standards page then the ticket moved to 9.0 for its final resolution.