fix(lineChart): prevent NullPointerException when lines is not configured#4919
fix(lineChart): prevent NullPointerException when lines is not configured#4919Ma77Ball wants to merge 5 commits intoapache:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Line Chart visualization operator to fail fast when no line series are configured, instead of crashing with a NullPointerException during Python code generation. It fits into the visualization operator code by aligning LineChartOpDesc's configuration handling with the validation style used by similar chart operators.
Changes:
- Initialize
LineChartOpDesc.lineswith an emptyArrayListinstead of leaving itnull. - Add a non-empty assertion in
createPlotlyFigure()before generating Plotly traces. - Update
LineChartOpDescSpecto expectAssertionErrorfor default and explicitly emptylinesconfigurations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/lineChart/LineChartOpDesc.scala |
Changes Line Chart config initialization and adds fail-fast validation before trace generation. |
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/lineChart/LineChartOpDescSpec.scala |
Updates unit tests to cover the new empty/default lines validation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@aglinxinyuan please review. |
| val configuredLines = Option(lines).getOrElse(new util.ArrayList[LineConfig]()) | ||
| assert(configuredLines.asScala.nonEmpty, "At least one line must be configured") | ||
| val linesPart = configuredLines.asScala |
There was a problem hiding this comment.
the default-value + null-guard + non-empty assertion is slightly belt-and-braces. Since lines is an external mutable field, the contract that matters is just "non-null and non-empty," which collapses to a single assertion at the top:
def createPlotlyFigure(): PythonTemplateBuilder = {
assert(lines != null && !lines.isEmpty, "At least one line must be configured")
val linesPart = lines.asScala.map { ... }That avoids the unused empty-list allocation on the null path and the double .asScala wrap. The current form is correct — just a bit easier to read this way.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4919 +/- ##
============================================
- Coverage 42.68% 42.48% -0.21%
+ Complexity 2183 2182 -1
============================================
Files 1031 1005 -26
Lines 38110 37426 -684
Branches 4004 3918 -86
============================================
- Hits 16266 15899 -367
+ Misses 20829 20558 -271
+ Partials 1015 969 -46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
What changes were proposed in this PR?
LineChartOpDesc declared var lines: util.List[LineConfig] = _, which defaults to null in Scala/Java. Calling generatePythonCode() on a freshly constructed instance immediately crashed with a NullPointerException at
createPlotlyFigure because .asScala.map(...) was called on a null reference.
Two changes:
Any related issues, documentation, or discussions?
#4811
How was this PR tested?
Unit tests in LineChartOpDescSpec cover:
Was this PR authored or co-authored using generative AI tooling?
Co-authored with Claude opus 4.6 in compliance with ASF