Re: [Bigdata-developers] MIN and MAX aggregates fixed: trac 736 (with question?)

[Jeremy J C. <jj...@sy...>]

I have verified that MIN and MAX both inherit from INeedsMaterialization
and added tests that use some of the literals from TestIVComparator that need materialization.

Unless Jenkins says otherwise I believe the change is good

Jeremy J Carroll
Principal Architect
Syapse, Inc.



On Oct 9, 2013, at 5:04 PM, Jeremy J Carroll <jj...@sy...> wrote:

> This is really helpful, I was just about to go home, but think I will spend a few more minutes on this!
> 
> 
> Jeremy J Carroll
> Principal Architect
> Syapse, Inc.
> 
> 
> 
> On Oct 9, 2013, at 5:00 PM, Bryan Thompson <br...@sy...> wrote:
> 
>> From the ticket you mentioned -- the INeedsMaterialization interface is
>> used to declare which bops require materialization.  This gets lifted from
>> subexpressions to decide if we need to do materialization before entering
>> the expression. 
>> 
>>>>> SNIP<<<
>> There are some known issues with comparisons of inline and non-inline
>> IVs which MikeP will be addressing shortly.  Basically, we need an
>> INeedsMaterialization option for operators which always require the
>> materialization of non-inline IVs.  ORDER_BY and aggregation both have
>> this characteristic since they must run over all solutions at once and
>> can not re-try solutions which have failed with a
>> NotMaterializedException?
>> <https://sourceforge.net/apps/trac/bigdata/wiki/NotMaterializedException>.
>> As an initial step in that direction, I have defined an IVComparator
>> and a test suite for that class and also added a
>> compareLiteral(IV,Literal) to IVUtility.  While this covers some cases,
>> it does not cover all as demonstrated by TestIVComparator.
>> 
>>>>> SNIP<<<
>> 
>> I think that the question for MIN and MAX is just whether they are
>> imposing the total ordering for SPARQL over RDF Values.  Again, I would
>> have to look at the code. That might be an old FIXME.  Or it might be a
>> valid problem.  However, I think that there are DAWG test cases that cover
>> this.  I would check there first and make sure that there is a problem (or
>> an absence of test coverage).
>> 
>> 
>> Bryan
>> 
>> 
>> On 10/9/13 7:26 PM, "Jeremy J Carroll" <jj...@sy...> wrote:
>> 
>>> 
>>> Please glance at this code (from MIN.java or MAX.java)
>>> 
>>> ...
>>> 
>>> +           private static IVComparator comparator = new IVComparator();
>>> 
>>> Š
>>> 
>>> 
>>> -                /**
>>> -                 * FIXME This needs to use the ordering define by
>>> ORDER_BY. The
>>> -                 * CompareBOp imposes the ordering defined for the "<"
>>> operator
>>> -                 * which is less robust and will throw a type exception
>>> if you
>>> -                 * attempt to compare unlike Values.
>>> -                 *
>>> -                 * @see
>>> https://sourceforge.net/apps/trac/bigdata/ticket/300#comment:5
>>> -                 */
>>> -                if (CompareBOp.compare(iv, min, CompareOp.LT)) {
>>> +                if (comparator.compare(iv, min)<0) {
>>> 
>>>                   min = iv;
>>> 
>>>               }
>>> 
>>> 
>>> 
>>> It seems to work, but I read something about requiring materialization
>>> which I did not understand and chose to ignore - was that a mistake?
>>> 
>>> Jeremy J Carroll
>>> Principal Architect
>>> Syapse, Inc.
>>> 
>>> 
>>> 
>>> 
>>> --------------------------------------------------------------------------
>>> ----
>>> October Webinars: Code for Performance
>>> Free Intel webinars can help you accelerate application performance.
>>> Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most
>>> from 
>>> the latest Intel processors and coprocessors. See abstracts and register >
>>> http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktr
>>> k
>>> _______________________________________________
>>> Bigdata-developers mailing list
>>> Big...@li...
>>> https://lists.sourceforge.net/lists/listinfo/bigdata-developers
>> 
>