Opened 3 months ago

Closed 2 months ago

#1596 closed defect (fixed)

WCPS1.5_Remove the check for applying trimming/slicing in dimensions interval

Reported by: bphamhuu Owned by: bphamhuu
Priority: major Milestone: 9.5
Component: petascope Version: development
Keywords: Cc: dmisev, vmerticariu
Complexity: Medium


In SubsetExpressionHandler? class, Vlad found that the else condition here is not correct and all cases should be handled by the first condition.

// NOTE: in case of coverage constant e.g: <[-1:1,-1:1],-1,0,1,....> it should not replace the interval inside the "< >"
        if (rasql.contains("<") || !rasql.contains("[")) {
            String dimensions = rasqlTranslationService.constructRasqlDomain(metadata.getSortedAxesByGridOrder(),
                                                        axisIteratorSubsetDimensions, axisIteratorAliasRegistry);
            rasqlSubset = template.replace("$dimensionIntervalList", dimensions);
        } else {
            // update the interval of the existing expression in template string
            // e.g: trim(c[0:5,0:100,89], {90:90}) -> c[0:5,90:90,89]
            // need to replace the interval correctly
            String tmp = rasql.substring(rasql.indexOf("[") + 1, rasql.indexOf("]"));
            String[] intervals = tmp.split(",");

            String axisName = subsetDimensions.get(0).getAxisName();
            Axis axis = metadata.getAxisByName(axisName);
            int axisOrder = axis.getRasdamanOrder();

            String dimension = rasqlTranslationService.constructRasqlDomain(ListUtil.valuesToList(axis), axisIteratorSubsetDimensions, axisIteratorAliasRegistry);
            intervals[axisOrder] = dimension;

            // 0:5,0:100,89
            String intervalsStr = "[" + StringUtils.join(intervals, ",") + "]";
            rasqlSubset = rasql.replaceAll("\\[.*?\\]", intervalsStr);

So if the tests for WCPS have no problem then can remove the if else condition.

Change History (2)

comment:1 Changed 3 months ago by vmerticariu

Just to give a little more context:

The else branch handles the cases of nested subsets, e.g.

(c[Lat(0:10), Long(0:10)])[Lat(0), Long(0)] will be translated into rasql in the same way as c[Lat(0), Long(0)], resulting in a single interval.

The problem is that it is assumed that the second subset is applied on the same metadata as the first one, basically overriding the first subset . This is not always the case, there can be an operation that changes the metadata:

avg(c[Lat(0:10), Long(0:10)])[Lat(0), Long(0)] will move the subset inside the avg, and will be translated to avg(c[Lat(0), Long(0)]) when in fact the outer subset is applicable to a different object, and should result in an error.

Since rasql supports nested subsets, I think we shouldn't attempt to rewrite the wcps query, and simply forward it to rasql with 2 subsets instead of merging them heuristically. This would be achieved by removing the if and keeping the first branch only.

comment:2 Changed 2 months ago by bphamhuu

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.