Changing the array behavior to start the index from 1. #1682
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,6 +54,7 @@ | |
| import io.confluent.ksql.parser.tree.SubscriptExpression; | ||
| import io.confluent.ksql.parser.tree.SymbolReference; | ||
| import io.confluent.ksql.util.ExpressionTypeManager; | ||
| import io.confluent.ksql.util.KsqlConfig; | ||
| import io.confluent.ksql.util.KsqlException; | ||
| import io.confluent.ksql.util.Pair; | ||
| import io.confluent.ksql.util.SchemaUtil; | ||
|
|
@@ -66,14 +67,19 @@ | |
|
|
||
| public class SqlToJavaVisitor { | ||
|
|
||
| private final Schema schema; | ||
| private final FunctionRegistry functionRegistry; | ||
| private final KsqlConfig ksqlConfig; | ||
|
|
||
| private final ExpressionTypeManager expressionTypeManager; | ||
|
|
||
| public SqlToJavaVisitor( | ||
| final Schema schema, | ||
| final FunctionRegistry functionRegistry, | ||
| final KsqlConfig ksqlConfig) { | ||
| this.schema = schema; | ||
| this.functionRegistry = functionRegistry; | ||
| this.ksqlConfig = ksqlConfig; | ||
| this.expressionTypeManager = | ||
| new ExpressionTypeManager(schema, functionRegistry); | ||
| } | ||
|
|
@@ -539,11 +545,12 @@ protected Pair<String, Schema> visitSubscriptExpression( | |
| switch (internalSchema.type()) { | ||
| case ARRAY: | ||
| return new Pair<>( | ||
| String.format("((%s) (ArrayGet.getItem(((%s) %s), (int)(%s), %s)))", | ||
|
There was a problem hiding this comment. If index is null, then this will throw NPE. The index may be derived from the source query e.g. So we may want to consider handling a null index by just returning null. (I've not checked what other vendors do, but most seem to err on the side of not-logging-loads-of-errors and just returning null) There was a problem hiding this comment. One problem with returning null is that we won't know if the value of the array item is null or there has been an error. We do return null for the column in case of exception anyway but I think having the error cause in the log is informative, although I agree that we may end up having lot's of errors in the log. There was a problem hiding this comment.
I'd normally agree with you. I'm a fan or fail hard and fail fast. However, the pattern for most SQL implementations is to be pretty lax about error reporting. For example, substring just returns null if the start index is out of bounds. At the very least, we should be logging a more informative error than an NPE. There was a problem hiding this comment. I commented in another thread, but I think it would be better form to log the exception explicitly at a debug level so that we can control what goes into our logs. There was a problem hiding this comment. If we throw an exception it will be logged in the processing log... |
||
| SchemaUtil.getJavaType(internalSchema.valueSchema()).getSimpleName(), | ||
| internalSchemaJavaType, | ||
| process(node.getBase(), unmangleNames).getLeft(), | ||
| process(node.getIndex(), unmangleNames).getLeft(), | ||
| ksqlConfig.getBoolean(KsqlConfig.KSQL_FUNCTIONS_ARRAY_LEGACY_BASE_CONFIG) | ||
| ), | ||
| internalSchema.valueSchema() | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /* | ||
| * Copyright 2018 Confluent Inc. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| **/ | ||
|
|
||
| package io.confluent.ksql.codegen.helpers; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| public final class ArrayGet { | ||
|
|
||
| private ArrayGet() {} | ||
|
|
||
| public static Object getItem(final List<?> array, final int index, final boolean isLegacy) { | ||
| final int correctIndex = isLegacy ? index : index - 1; | ||
| return array.get(correctIndex); | ||
|
There was a problem hiding this comment. You've removed the out of bounds handing that was here before. Just wondering why? I'm guessing this is inline with your comment above, and you want the exception to be logged? If so, then I think this should be a general question to the team to come to a decision on how we handle such issues: do we silently handle them and return null, or do we throw an exception, log it and then return null? Personally, I'm with you and prefer the logging approach. But if this is not what expected by our users then they might not thank us for spamming the logs. Likewise, in cloud we're going to end up with a lot of noise in the logs, (though we can turn it off I guess). Which ever way we decide to go we should be consistent. So, for example, if we go with throwing exceptions, then we should change There was a problem hiding this comment. I think for now we can throw an exception and return null. This way we make sure we don't lose information that user may need. This is the same behavior for other types of expressions. Let's make There was a problem hiding this comment. I am not a fan of just dumping exception traces to the log by default. Even if we have #1795, it would be much better to explicitly log this at some sort of verbose logging level (like debug) so that we can control what is logged by setting the class level log level in the config. If these types of things were logged at debug level, then in most cases we won't see them until we want to debug something, at which point we can change the log level for the class (potentially at run time through JMX) and then collect the necessary information. If you see our soak cluster, we already have many NPE stack traces in the logs. These are really hard to work with because:
I vote for not adding to the problem in this patch. |
||
| } | ||
|
|
||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we just name this
ksql.functions.array.zero.base. It seems like a much more self explanatory name.