You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metron.apache.org by jjmeyer0 <gi...@git.apache.org> on 2016/12/07 03:37:24 UTC
[GitHub] incubator-metron pull request #390: METRON-364: Preserve Type for Arithmetic...
GitHub user jjmeyer0 opened a pull request:
https://github.com/apache/incubator-metron/pull/390
METRON-364: Preserve Type for Arithmetic Expressions in Stellar
updated stellar arithmetic expressions to be evaluated in a manner that keeps type. For example, if two integers are added then an integer will be returned.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jjmeyer0/incubator-metron METRON-364
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-metron/pull/390.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #390
----
commit b69dabe5a50e5c32b8e44f03db187894199fe5a4
Author: JJ <jj...@gmail.com>
Date: 2016-12-07T02:50:40Z
METRON-364: updated stellar arithmetic expressions to be evaluated in a manner that keeps type. For example, if two integers are added then an integer will be returned.
commit dce230124ec62473e0102dc144a7ae911de13ca6
Author: JJ <jj...@gmail.com>
Date: 2016-12-07T03:28:42Z
METRON-364: updated stellar arithmetic expressions to be evaluated in a manner that keeps type. For example, if two integers are added then an integer will be returned.
commit cdd69dc3a90f0e35a117248980588179c4e3ed5c
Author: JJ <jj...@gmail.com>
Date: 2016-12-07T03:34:35Z
Uncommenting tests that should still run.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #390: METRON-364: Preserve Type for Arithmetic Expres...
Posted by jjmeyer0 <gi...@git.apache.org>.
Github user jjmeyer0 commented on the issue:
https://github.com/apache/incubator-metron/pull/390
The reason I neglected longs and floats is because I didn't see the Stellar dsl having the ability to represent them. For example, the test below fails with a parse exception:
`
@Test
public void testLongAddition() throws Exception {
String query = "2147483650 + 2";
Assert.assertEquals(2147483652L, run(query, new HashMap<>()));
}
`
That being said it makes sense to still handle longs and floats, if Stellar will at some point. If not that, at the very least if this evaluator sees a long or float it should error out.
Should I add support for longs/floats and then create a Jira for Stellar to add them? Or am I mistaken and Stellar already has support?
Thanks for the feedback.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #390: METRON-364: Preserve Type for Arithmetic Expres...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/390
Oops also we will want to preserve Longs. Down converting to an int will lose precision and may end up being inaccurate. I'd suggest we operate like so:
* If either are a double then operate as doubles
* else if either are float then operate as floats
* else if either are longs then operate as longs
* else operate as ints
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #390: METRON-364: Preserve Type for Arithmetic Expres...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/incubator-metron/pull/390
[moving this to unbury it]
My only question here, and I may be out of my depth, is does this functionality match the behavior of java itself? Or other languages? Would the result of operations on mixed types be consistent (rule of least surprise)? For example:
Float + Long = Float
Long + Float = Long
Hopefully this makes sense
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron pull request #390: METRON-364: Preserve Type for Arithmetic...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/390#discussion_r91225030
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/stellar/evaluators/ArithmeticEvaluator.java ---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.metron.common.stellar.evaluators;
+
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.metron.common.dsl.Token;
+
+import java.util.function.BiFunction;
+
+public class ArithmeticEvaluator {
+
+ public Token<? extends Number> evaluate(BiFunction<Number, Number, Token<? extends Number>> function,
+ Pair<Token<? extends Number>, Token<? extends Number>> p) {
+ if (p == null || p.getKey() == null || p.getValue() == null) {
+ throw new IllegalArgumentException();
+ }
+
+ final Number l = p.getKey().getValue();
+ final Number r = p.getValue().getValue();
+
+ return function.apply(l == null ? 0 : l, r == null ? 0 : r);
+ }
+
+ public interface ArithmeticEvaluatorFunctions {
+ static BiFunction<Number, Number, Token<? extends Number>> addition() {
+ return (Number l, Number r) -> {
+ if (l instanceof Double || r instanceof Double) {
--- End diff --
What about if l or r is Float?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron pull request #390: METRON-364: Preserve Type for Arithmetic...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/390#discussion_r91386552
--- Diff: metron-platform/metron-common/src/test/java/org/apache/metron/common/stellar/StellarTest.java ---
@@ -56,6 +56,37 @@
public class StellarTest {
@Test
+ public void addingLongsShouldYieldLong() throws Exception {
+ final long timestamp = 1452013350000L;
+ String query = "TO_EPOCH_TIMESTAMP('2016-01-05 17:02:30', 'yyyy-MM-dd HH:mm:ss', 'UTC') + 2";
+ Assert.assertEquals(timestamp + 2, run(query, new HashMap<>()));
+ }
+
+ @Test
+ public void addingIntegersShouldYieldAnInteger() throws Exception {
+ String query = "1 + 2";
+ Assert.assertEquals(3, run(query, new HashMap<>()));
+ }
+
+ @Test
+ public void addingDoublesShouldYieldADouble() throws Exception {
+ String query = "1.0 + 2.0";
+ Assert.assertEquals(3.0, run(query, new HashMap<>()));
+ }
+
+ @Test
+ public void addingDoubleAndIntegerWhereSubjectIsDoubleShouldYieldADouble() throws Exception {
+ String query = "2.1 + 1";
+ Assert.assertEquals(3.1, run(query, new HashMap<>()));
+ }
+
+ @Test
+ public void addingDoubleAndIntegerWhereSubjectIsIntegerShouldYieldADouble() throws Exception {
+ String query = "1 + 2.1";
+ Assert.assertEquals(3.1, run(query, new HashMap<>()));
+ }
--- End diff --
Could we ensure that we have the following test cases and they return the proper types:
* float + long returns a float
* long + float returns a float
* double + int returns a double
* int + double returns a double
* float + double returns a double
* double + float returns a double
* long + double returns a double
* double + long returns a double
* long + int returns a long
* int + long returns a long
* int + int returns int
* float + float returns float
* double + double returns double
* long + long returns long
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron pull request #390: METRON-364: Preserve Type for Arithmetic...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/390#discussion_r91391147
--- Diff: metron-platform/metron-common/src/test/java/org/apache/metron/common/stellar/StellarTest.java ---
@@ -56,6 +56,37 @@
public class StellarTest {
@Test
+ public void addingLongsShouldYieldLong() throws Exception {
+ final long timestamp = 1452013350000L;
+ String query = "TO_EPOCH_TIMESTAMP('2016-01-05 17:02:30', 'yyyy-MM-dd HH:mm:ss', 'UTC') + 2";
+ Assert.assertEquals(timestamp + 2, run(query, new HashMap<>()));
+ }
+
+ @Test
+ public void addingIntegersShouldYieldAnInteger() throws Exception {
+ String query = "1 + 2";
+ Assert.assertEquals(3, run(query, new HashMap<>()));
+ }
+
+ @Test
+ public void addingDoublesShouldYieldADouble() throws Exception {
+ String query = "1.0 + 2.0";
+ Assert.assertEquals(3.0, run(query, new HashMap<>()));
+ }
+
+ @Test
+ public void addingDoubleAndIntegerWhereSubjectIsDoubleShouldYieldADouble() throws Exception {
+ String query = "2.1 + 1";
+ Assert.assertEquals(3.1, run(query, new HashMap<>()));
+ }
+
+ @Test
+ public void addingDoubleAndIntegerWhereSubjectIsIntegerShouldYieldADouble() throws Exception {
+ String query = "1 + 2.1";
+ Assert.assertEquals(3.1, run(query, new HashMap<>()));
+ }
--- End diff --
@ottobackwards Definitely; agreed. We should have a `StellarArithmeticTest` where we have this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #390: METRON-364: Preserve Type for Arithmetic Expres...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/390
@ottobackwards Yeah, that was my concern too. The suggestion I made was based around the Java spec (http://docs.oracle.com/javase/specs/jls/se8/html/jls-5.html#jls-5.6.2 as @justinleet linked earlier). This is intended to make it follow the rule of least surprise (I think ;). In your example, Long + Float should be Float by my suggestion.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron pull request #390: METRON-364: Preserve Type for Arithmetic...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/390#discussion_r91364920
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/stellar/evaluators/ArithmeticEvaluator.java ---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.metron.common.stellar.evaluators;
+
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.metron.common.dsl.Token;
+
+import java.util.function.BiFunction;
+
+public class ArithmeticEvaluator {
+
+ public Token<? extends Number> evaluate(BiFunction<Number, Number, Token<? extends Number>> function,
+ Pair<Token<? extends Number>, Token<? extends Number>> p) {
+ if (p == null || p.getKey() == null || p.getValue() == null) {
+ throw new IllegalArgumentException();
+ }
+
+ final Number l = p.getKey().getValue();
+ final Number r = p.getValue().getValue();
+
+ return function.apply(l == null ? 0 : l, r == null ? 0 : r);
+ }
+
+ public interface ArithmeticEvaluatorFunctions {
+ static BiFunction<Number, Number, Token<? extends Number>> addition() {
+ return (Number l, Number r) -> {
+ if (l instanceof Double || r instanceof Double) {
--- End diff --
My only question here, and I may be out of my depth, is does this functionality match the behavior of java itself? Or other languages? Would the result of operations on mixed types be consistent (rule of least surprise)? For example ( and again I'm just imagining here ) Float + Long = Float. Long + Float = Long.
Hopefully this makes sense
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron pull request #390: METRON-364: Preserve Type for Arithmetic...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-metron/pull/390
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #390: METRON-364: Preserve Type for Arithmetic Expres...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/incubator-metron/pull/390
I agree on operating Casey's rules, given that they match Java's rules for handling this.
http://docs.oracle.com/javase/specs/jls/se8/html/jls-5.html#jls-5.6.2
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #390: METRON-364: Preserve Type for Arithmetic Expres...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/390
Cool! Dig the tests. +1 by inspection
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #390: METRON-364: Preserve Type for Arithmetic Expres...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/incubator-metron/pull/390
@cestella I see it now, but not when I made the comment. Sorry for adding confusion.
+1 by inspection
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #390: METRON-364: Preserve Type for Arithmetic Expres...
Posted by jjmeyer0 <gi...@git.apache.org>.
Github user jjmeyer0 commented on the issue:
https://github.com/apache/incubator-metron/pull/390
I believe I have updated all tests and addressed each of the concerns. Thanks again for all the help!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #390: METRON-364: Preserve Type for Arithmetic Expres...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/390
You can return them from as results from stellar functions. Longs are
output from time stamp functions for instance.
On Wed, Dec 7, 2016 at 08:00 JJ Meyer <no...@github.com> wrote:
> The reason I neglected longs and floats is because I didn't see the
> Stellar dsl having the ability to represent them. For example, the test
> below fails with a parse exception:
>
> @Test public void testLongAddition() throws Exception { String query =
> "2147483650 + 2"; Assert.assertEquals(2147483652L, run(query, new
> HashMap<>())); }
>
> That being said it makes sense to still handle longs and floats, if
> Stellar will at some point. If not that, at the very least if this
> evaluator sees a long or float it should error out.
>
> Should I add support for longs/floats and then create a Jira for Stellar
> to add them? Or am I mistaken and Stellar already has support?
>
> Thanks for the feedback.
>
> \u2014
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/incubator-metron/pull/390#issuecomment-265441115>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAg-x1540F94nUQNERbBY0OzYTPJKw9fks5rFq3bgaJpZM4LGKYJ>
> .
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron pull request #390: METRON-364: Preserve Type for Arithmetic...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/390#discussion_r91386719
--- Diff: metron-platform/metron-common/src/test/java/org/apache/metron/common/stellar/StellarTest.java ---
@@ -56,6 +56,37 @@
public class StellarTest {
@Test
+ public void addingLongsShouldYieldLong() throws Exception {
+ final long timestamp = 1452013350000L;
+ String query = "TO_EPOCH_TIMESTAMP('2016-01-05 17:02:30', 'yyyy-MM-dd HH:mm:ss', 'UTC') + 2";
+ Assert.assertEquals(timestamp + 2, run(query, new HashMap<>()));
+ }
+
+ @Test
+ public void addingIntegersShouldYieldAnInteger() throws Exception {
+ String query = "1 + 2";
+ Assert.assertEquals(3, run(query, new HashMap<>()));
+ }
+
+ @Test
+ public void addingDoublesShouldYieldADouble() throws Exception {
+ String query = "1.0 + 2.0";
+ Assert.assertEquals(3.0, run(query, new HashMap<>()));
+ }
+
+ @Test
+ public void addingDoubleAndIntegerWhereSubjectIsDoubleShouldYieldADouble() throws Exception {
+ String query = "2.1 + 1";
+ Assert.assertEquals(3.1, run(query, new HashMap<>()));
+ }
+
+ @Test
+ public void addingDoubleAndIntegerWhereSubjectIsIntegerShouldYieldADouble() throws Exception {
+ String query = "1 + 2.1";
+ Assert.assertEquals(3.1, run(query, new HashMap<>()));
+ }
--- End diff --
Obviously, the same set for multiplication and division. I'd parameterize that case ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron pull request #390: METRON-364: Preserve Type for Arithmetic...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/390#discussion_r91390855
--- Diff: metron-platform/metron-common/src/test/java/org/apache/metron/common/stellar/StellarTest.java ---
@@ -56,6 +56,37 @@
public class StellarTest {
@Test
+ public void addingLongsShouldYieldLong() throws Exception {
+ final long timestamp = 1452013350000L;
+ String query = "TO_EPOCH_TIMESTAMP('2016-01-05 17:02:30', 'yyyy-MM-dd HH:mm:ss', 'UTC') + 2";
+ Assert.assertEquals(timestamp + 2, run(query, new HashMap<>()));
+ }
+
+ @Test
+ public void addingIntegersShouldYieldAnInteger() throws Exception {
+ String query = "1 + 2";
+ Assert.assertEquals(3, run(query, new HashMap<>()));
+ }
+
+ @Test
+ public void addingDoublesShouldYieldADouble() throws Exception {
+ String query = "1.0 + 2.0";
+ Assert.assertEquals(3.0, run(query, new HashMap<>()));
+ }
+
+ @Test
+ public void addingDoubleAndIntegerWhereSubjectIsDoubleShouldYieldADouble() throws Exception {
+ String query = "2.1 + 1";
+ Assert.assertEquals(3.1, run(query, new HashMap<>()));
+ }
+
+ @Test
+ public void addingDoubleAndIntegerWhereSubjectIsIntegerShouldYieldADouble() throws Exception {
+ String query = "1 + 2.1";
+ Assert.assertEquals(3.1, run(query, new HashMap<>()));
+ }
--- End diff --
Casey - just like with the stringfunctionstest - I'm not sure that all of this should be in stellartest. How do you feel about breaking it out?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---