[
https://issues.apache.org/jira/browse/SPARK-58437?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Yang Jie updated SPARK-58437:
-----------------------------
Description:
Several codegen templates emit Java source that is not valid Java. Janino
accepts it
because it erases generics and does not enforce all of the rules javac does, so
these have
gone unnoticed. One of them has a real runtime consequence independent of which
compiler
compiles the generated code.
*1. A {{final}} local variable is reassigned*
{{Sequence.doGenCode}} declares {{final $arrElemType[] $arr = null;}} and then
passes
{{$arr}} to {{{}impl.genCode{}}}, which assigns to it. Assigning to a {{final}}
local is
illegal Java; Janino does not enforce it.
*2. Binary inner-class names in source position*
{{{}ArrayDistinct{}}}, {{{}ArrayUnion{}}}, {{ArrayIntersect}} and
{{ArrayExcept}} emit
{{{}scala.collection.mutable.ArrayBuilder$ofInt{}}}. {{$}} is the JVM binary
name separator,
not source syntax; the source form is {{{}ArrayBuilder.ofInt{}}}. Janino
resolves the binary
name directly.
*3. A field declared with a type argument that does not match what is assigned*
{{SortExec.doProduce}} declares the mutable state as
{{scala.collection.Iterator<UnsafeRow>}} and assigns
{{UnsafeExternalRowSorter.sort()}} to
it, which returns {{{}Iterator<InternalRow>{}}}. The rows are in fact
{{UnsafeRow}} - the
consumer casts each one back - so this is not a runtime defect, but the
declared type
argument is wrong.
*4. Generic array creation*
{{CodegenContext.declareMutableStates}} compacts same-typed mutable states into
an array,
emitting {{new Foo<X>[n]}} when the type is parameterized. Java forbids generic
array
creation; the idiom is {{{}Foo<X>[] a = new Foo[n]{}}}. Reachable today through
{{SampleExec.doConsume}} without replacement, whose
{{BernoulliCellSampler<UnsafeRow>}}
state is not force-inlined and therefore takes the array-compaction path.
Fixes 2-5 are no-ops under Janino; fix 1 corrects the parameter map's value
type. All five
are also prerequisites for compiling generated code with a stricter Java
compiler
(SPARK-57370).
Out of scope, noted here for the record: for the same condition, the
interpreted path
({{{}Sequence.getSequenceLength{}}}) throws a plain
{{IllegalArgumentException}} via
{{require}} with no error class, while the codegen path throws a
{{SparkIllegalArgumentException}} carrying {{{}_LEGACY_ERROR_TEMP_3243{}}}.
Reconciling the
two would change a user-visible exception type and belongs in its own change.
was:
Several codegen templates emit Java source that is not valid Java. Janino
accepts it
because it erases generics and does not enforce all of the rules javac does, so
these have
gone unnoticed. One of them has a real runtime consequence independent of which
compiler
compiles the generated code.
*1. A non-String value is stored into a {{Map<String, String>}}*
{{Sequence.genSequenceLengthCode}} builds the message parameters for
{{_LEGACY_ERROR_TEMP_3243}}:
{code:java}
java.util.Map<String, String> params = new java.util.HashMap<String, String>();
params.put("start", $start); // $start is a numeric expression
params.put("stop", $stop);
params.put("step", $step); // may be a CalendarInterval reference
{code}
Janino erases the type arguments and binds these to {{put(Object, Object)}}, so
numeric
values are autoboxed and, at the {{InternalSequenceBase}} call site, {{$step}}
is an
interval object. The map ends up holding {{java.lang.Long}} /
{{CalendarInterval}} values.
It reaches {{SparkIllegalArgumentException}} and is handed back by
{{SparkThrowable.getMessageParameters()}}, whose declared type is
{{java.util.Map<String, String>}} - a caller reading a value as {{String}} gets
a
{{ClassCastException}}. The rendered message text happens to be correct because
substitution calls {{toString}}. This error path currently has no test coverage.
*2. A {{final}} local variable is reassigned*
{{Sequence.doGenCode}} declares {{final $arrElemType[] $arr = null;}} and then
passes
{{$arr}} to {{impl.genCode}}, which assigns to it. Assigning to a {{final}}
local is
illegal Java; Janino does not enforce it.
*3. Binary inner-class names in source position*
{{ArrayDistinct}}, {{ArrayUnion}}, {{ArrayIntersect}} and {{ArrayExcept}} emit
{{scala.collection.mutable.ArrayBuilder$ofInt}}. {{$}} is the JVM binary name
separator,
not source syntax; the source form is {{ArrayBuilder.ofInt}}. Janino resolves
the binary
name directly.
*4. A field declared with a type argument that does not match what is assigned*
{{SortExec.doProduce}} declares the mutable state as
{{scala.collection.Iterator<UnsafeRow>}} and assigns
{{UnsafeExternalRowSorter.sort()}} to
it, which returns {{Iterator<InternalRow>}}. The rows are in fact {{UnsafeRow}}
- the
consumer casts each one back - so this is not a runtime defect, but the
declared type
argument is wrong.
*5. Generic array creation*
{{CodegenContext.declareMutableStates}} compacts same-typed mutable states into
an array,
emitting {{new Foo<X>[n]}} when the type is parameterized. Java forbids generic
array
creation; the idiom is {{Foo<X>[] a = new Foo[n]}}. Reachable today through
{{SampleExec.doConsume}} without replacement, whose
{{BernoulliCellSampler<UnsafeRow>}}
state is not force-inlined and therefore takes the array-compaction path.
Fixes 2-5 are no-ops under Janino; fix 1 corrects the parameter map's value
type. All five
are also prerequisites for compiling generated code with a stricter Java
compiler
(SPARK-57370).
Out of scope, noted here for the record: for the same condition, the
interpreted path
({{Sequence.getSequenceLength}}) throws a plain {{IllegalArgumentException}} via
{{require}} with no error class, while the codegen path throws a
{{SparkIllegalArgumentException}} carrying {{_LEGACY_ERROR_TEMP_3243}}.
Reconciling the
two would change a user-visible exception type and belongs in its own change.
> Fix invalid Java constructs in generated code that Janino tolerates
> -------------------------------------------------------------------
>
> Key: SPARK-58437
> URL: https://issues.apache.org/jira/browse/SPARK-58437
> Project: Spark
> Issue Type: Sub-task
> Components: SQL
> Affects Versions: 4.3.0, 5.0.0
> Reporter: Yang Jie
> Priority: Major
> Labels: pull-request-available
>
> Several codegen templates emit Java source that is not valid Java. Janino
> accepts it
> because it erases generics and does not enforce all of the rules javac does,
> so these have
> gone unnoticed. One of them has a real runtime consequence independent of
> which compiler
> compiles the generated code.
> *1. A {{final}} local variable is reassigned*
> {{Sequence.doGenCode}} declares {{final $arrElemType[] $arr = null;}} and
> then passes
> {{$arr}} to {{{}impl.genCode{}}}, which assigns to it. Assigning to a
> {{final}} local is
> illegal Java; Janino does not enforce it.
> *2. Binary inner-class names in source position*
> {{{}ArrayDistinct{}}}, {{{}ArrayUnion{}}}, {{ArrayIntersect}} and
> {{ArrayExcept}} emit
> {{{}scala.collection.mutable.ArrayBuilder$ofInt{}}}. {{$}} is the JVM binary
> name separator,
> not source syntax; the source form is {{{}ArrayBuilder.ofInt{}}}. Janino
> resolves the binary
> name directly.
> *3. A field declared with a type argument that does not match what is
> assigned*
> {{SortExec.doProduce}} declares the mutable state as
> {{scala.collection.Iterator<UnsafeRow>}} and assigns
> {{UnsafeExternalRowSorter.sort()}} to
> it, which returns {{{}Iterator<InternalRow>{}}}. The rows are in fact
> {{UnsafeRow}} - the
> consumer casts each one back - so this is not a runtime defect, but the
> declared type
> argument is wrong.
> *4. Generic array creation*
> {{CodegenContext.declareMutableStates}} compacts same-typed mutable states
> into an array,
> emitting {{new Foo<X>[n]}} when the type is parameterized. Java forbids
> generic array
> creation; the idiom is {{{}Foo<X>[] a = new Foo[n]{}}}. Reachable today
> through
> {{SampleExec.doConsume}} without replacement, whose
> {{BernoulliCellSampler<UnsafeRow>}}
> state is not force-inlined and therefore takes the array-compaction path.
> Fixes 2-5 are no-ops under Janino; fix 1 corrects the parameter map's value
> type. All five
> are also prerequisites for compiling generated code with a stricter Java
> compiler
> (SPARK-57370).
> Out of scope, noted here for the record: for the same condition, the
> interpreted path
> ({{{}Sequence.getSequenceLength{}}}) throws a plain
> {{IllegalArgumentException}} via
> {{require}} with no error class, while the codegen path throws a
> {{SparkIllegalArgumentException}} carrying {{{}_LEGACY_ERROR_TEMP_3243{}}}.
> Reconciling the
> two would change a user-visible exception type and belongs in its own change.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
