flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gábor Gévay <gga...@gmail.com>
Subject Re: [DISCUSS] Make FieldAccessor logic consistent with remaining API
Date Thu, 27 Oct 2016 12:52:18 GMT
Hello,

Thanks Fabian for starting this discussion.

I would just like to add a few thougths about why does the
FieldAccessors even exist. One could say that we should instead just
re-use ExpressionKeys for the aggregations, and we are done. As far as
I can see, the way ExpressionKeys is often used is roughly to call
computeLogicalKeyPositions on it, then call
CompositeType.createComparator with these key positions, and then we
can call extractKeys on the comparator to get the values of the fields
of a record.

However, the problem with this is that in the aggregations we also
need to write fields and not just read them. So with the above
procedure, we would need an "intractKey" method on the comparators
that would be the opposite of extractKeys, i.e. which would set the
key fields.

Another difference between the FieldAccessor world and the
ExpressionKeys world is that in the aggregations we need to deal with
only one field. This means that enhancing the comparators with the
exact opposite of extractKeys would be overkill, because we don't need
to set multiple fields at a time.

It certainly seems to be a good idea to unify these at least
externally (from the side of the users), but I'm not sure what would
be a neat way to also internally unify them.

Best,
Gábor


2016-10-26 20:54 GMT+02:00 Fabian Hueske <fhueske@gmail.com>:
> Hi everybody,
>
> while reviewing PR #2094 [1] I noticed that the field reference syntax for
> FieldAccessors is not compatible with the syntax supported for key
> definitions (ExpressionKeys) used in groupBy(), keyBy(),
> join().where().equalTo(), etc.
>
> FieldAccessors are only used for build-in aggregations in the DataStream
> API (sum(), min(), max(), ...).
>
> In particular I identified the following inconsistencies:
>
> - FieldAccessors allow to address array cells. ExpressionKeys treat arrays
> as AtomicTypes (Array TypeInfos do not extend CompositeType). Hence, it is
> not possible to address array cells.
> - ExpressionKeys do only support Integer keys for tuples. An atomic type
> can only be addressed with "*". FieldAccessors allow to address AtomicTypes
> with 0 in addtion to "*".
> - ExpressionKeys support to address fields of Java tuples with "f2" and
> Scala tuple fields with "_3". FieldAccessors do not support the "f" or "_"
> prefix.
>
> I would like to propose to adapt the syntax of both mechanisms (ideally,
> both should use the same code for validation). IMO, the ExpressionKey
> syntax much more widely used and is well designed. Therefore, I would adopt
> it for FieldAccessors as well. However, that would mean to restrict the
> syntax of the FieldAccessors and might break existing code.
>
> What do others think?
>
> [1] https://github.com/apache/flink/pull/2094

Mime
View raw message