Re: spark git commit: [SPARK-15204][SQL] improve nullability inference for Aggregator

[Reynold Xin]

Jacek,

This is definitely not necessary, but I wouldn't waste cycles "fixing"
things like this when they have virtually zero impact. Perhaps next time we
update this code we can "fix" it.

Also can you comment on the pull request directly?


On Tue, Jul 5, 2016 at 1:07 PM, Jacek Laskowski  wrote:

> On Mon, Jul 4, 2016 at 6:14 AM,   wrote:
> > Repository: spark
> > Updated Branches:
> >   refs/heads/master 88134e736 -> 8cdb81fa8
> >
> >
> > [SPARK-15204][SQL] improve nullability inference for Aggregator
> >
> > ## What changes were proposed in this pull request?
> >
> > TypedAggregateExpression sets nullable based on the schema of the
> outputEncoder
> >
> > ## How was this patch tested?
> >
> > Add test in DatasetAggregatorSuite
> >
> > Author: Koert Kuipers 
> ...
> > +assert(ds1.select(typed.sum((i: Int) => i)).schema.head.nullable
> === false)
> > +val ds2 = Seq(AggData(1, "a"), AggData(2, "a")).toDS()
> > +assert(ds2.select(SeqAgg.toColumn).schema.head.nullable === true)
> > +val ds3 = sql("SELECT 'Some String' AS b, 1279869254 AS
> a").as[AggData]
> > +assert(ds3.select(NameAgg.toColumn).schema.head.nullable === true)
>
> Why do we assert predicates? If it's true, it's true already (no need
> to compare whether it's true or not). I'd vote to "fix" it.
>
> Jacek
>
> -
> To unsubscribe e-mail: dev-unsubscr...@spark.apache.org
>
>

Re: spark git commit: [SPARK-15204][SQL] improve nullability inference for Aggregator

[Koert Kuipers]

oh you mean instead of:
assert(ds3.select(NameAgg.toColumn).schema.head.nullable === true)
just do:
assert(ds3.select(NameAgg.toColumn).schema.head.nullable)

i did mostly === true because i also had === false, and i liked the
symmetry, but sure this can be fixed if its not the norm

On Tue, Jul 5, 2016 at 4:07 PM, Jacek Laskowski  wrote:

> On Mon, Jul 4, 2016 at 6:14 AM,   wrote:
> > Repository: spark
> > Updated Branches:
> >   refs/heads/master 88134e736 -> 8cdb81fa8
> >
> >
> > [SPARK-15204][SQL] improve nullability inference for Aggregator
> >
> > ## What changes were proposed in this pull request?
> >
> > TypedAggregateExpression sets nullable based on the schema of the
> outputEncoder
> >
> > ## How was this patch tested?
> >
> > Add test in DatasetAggregatorSuite
> >
> > Author: Koert Kuipers 
> ...
> > +assert(ds1.select(typed.sum((i: Int) => i)).schema.head.nullable
> === false)
> > +val ds2 = Seq(AggData(1, "a"), AggData(2, "a")).toDS()
> > +assert(ds2.select(SeqAgg.toColumn).schema.head.nullable === true)
> > +val ds3 = sql("SELECT 'Some String' AS b, 1279869254 AS
> a").as[AggData]
> > +assert(ds3.select(NameAgg.toColumn).schema.head.nullable === true)
>
> Why do we assert predicates? If it's true, it's true already (no need
> to compare whether it's true or not). I'd vote to "fix" it.
>
> Jacek
>
> -
> To unsubscribe e-mail: dev-unsubscr...@spark.apache.org
>
>

Re: spark git commit: [SPARK-15204][SQL] improve nullability inference for Aggregator

[Jacek Laskowski]

On Mon, Jul 4, 2016 at 6:14 AM,   wrote:
> Repository: spark
> Updated Branches:
>   refs/heads/master 88134e736 -> 8cdb81fa8
>
>
> [SPARK-15204][SQL] improve nullability inference for Aggregator
>
> ## What changes were proposed in this pull request?
>
> TypedAggregateExpression sets nullable based on the schema of the 
> outputEncoder
>
> ## How was this patch tested?
>
> Add test in DatasetAggregatorSuite
>
> Author: Koert Kuipers 
...
> +assert(ds1.select(typed.sum((i: Int) => i)).schema.head.nullable === 
> false)
> +val ds2 = Seq(AggData(1, "a"), AggData(2, "a")).toDS()
> +assert(ds2.select(SeqAgg.toColumn).schema.head.nullable === true)
> +val ds3 = sql("SELECT 'Some String' AS b, 1279869254 AS a").as[AggData]
> +assert(ds3.select(NameAgg.toColumn).schema.head.nullable === true)

Why do we assert predicates? If it's true, it's true already (no need
to compare whether it's true or not). I'd vote to "fix" it.

Jacek

-
To unsubscribe e-mail: dev-unsubscr...@spark.apache.org