You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/09/24 00:40:52 UTC
[GitHub] [cassandra] xvade opened a new pull request, #1878: Math fcts
xvade opened a new pull request, #1878:
URL: https://github.com/apache/cassandra/pull/1878
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-17221: Math fcts [cassandra]
Posted by "pmcfadin (via GitHub)" <gi...@apache.org>.
pmcfadin commented on PR #1878:
URL: https://github.com/apache/cassandra/pull/1878#issuecomment-4227144375
Closing as [CASSANDRA-17221](https://issues.apache.org/jira/browse/CASSANDRA-17221) has been resolved as Fixed — if this PR contains changes not captured in the merged fix, please reopen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] xvade commented on pull request #1878: CASSANDRA-17221: Math fcts
Posted by GitBox <gi...@apache.org>.
xvade commented on PR #1878:
URL: https://github.com/apache/cassandra/pull/1878#issuecomment-1263045812
> hi @XV4DE , what is the jira ticket this patch is for? Thanks.
@smiklosovic https://issues.apache.org/jira/browse/CASSANDRA-17221
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blerer commented on pull request #1878: CASSANDRA-17221: Math fcts
Posted by GitBox <gi...@apache.org>.
blerer commented on PR #1878:
URL: https://github.com/apache/cassandra/pull/1878#issuecomment-1271573281
What I had in mind was something like:
```
@Test
public void testAbs()
{
assertAbsEquals(1, Int32Type.instance, 1);
assertAbsEquals(0, Int32Type.instance, 0);
assertAbsEquals(3, Int32Type.instance, -3);
assertAbsEquals((byte) 1, ByteType.instance, (byte) 1);
assertAbsEquals((byte) 0, ByteType.instance, (byte) 0);
assertAbsEquals((byte) 3, ByteType.instance, (byte) -3);
assertAbsEquals((short) 1, ShortType.instance, (short) 1);
assertAbsEquals((short) 0, ShortType.instance, (short) 0);
assertAbsEquals((short) 3, ShortType.instance, (short) -3);
assertAbsEquals(1F, FloatType.instance, 1F);
assertAbsEquals(1.5F, FloatType.instance, 1.5F);
assertAbsEquals(0F, FloatType.instance, 0F);
assertAbsEquals(3F, FloatType.instance, -3F);
assertAbsEquals(3.7F, FloatType.instance, -3.7F);
for (AbstractType<Long> type : new AbstractType[]{LongType.instance, CounterColumnType.instance})
{
assertAbsEquals(1L, LongType.instance, 1L);
assertAbsEquals(0L, LongType.instance, 0L);
assertAbsEquals(3L, LongType.instance, -3L);
}
assertAbsEquals(1.0, DoubleType.instance, 1.0);
assertAbsEquals(1.5, DoubleType.instance, 1.5);
assertAbsEquals(0.0, DoubleType.instance, 0.0);
assertAbsEquals(3.0, DoubleType.instance, -3.0);
assertAbsEquals(3.7, DoubleType.instance, -3.7);
assertAbsEquals(BigInteger.ONE, IntegerType.instance, BigInteger.ONE);
assertAbsEquals(BigInteger.ZERO, IntegerType.instance, BigInteger.ZERO);
assertAbsEquals(BigInteger.valueOf(3), IntegerType.instance, BigInteger.valueOf(-3));
assertAbsEquals(BigDecimal.ONE, DecimalType.instance, BigDecimal.ONE);
assertAbsEquals(BigDecimal.valueOf(1.5), DecimalType.instance, BigDecimal.valueOf(1.5));
assertAbsEquals(BigDecimal.ZERO, DecimalType.instance, BigDecimal.ZERO);
assertAbsEquals(BigDecimal.valueOf(3), DecimalType.instance, BigDecimal.valueOf(-3));
assertAbsEquals(BigDecimal.valueOf(3.7), DecimalType.instance, BigDecimal.valueOf(-3.7));
}
private <T extends Number> void assertAbsEquals(T expected, NumberType<T> inputType, T inputValue)
{
assertFctEquals(MathFcts.absFct(inputType), expected, inputType, inputValue);
}
private <T extends Number> void assertFctEquals(ScalarFunction fct, T expected, NumberType<T> inputType, T inputValue)
{
ByteBuffer input = inputType.decompose(inputValue);
assertEquals(expected, inputType.compose(executeFunction(fct, input)));
}
```
It makes the test a bit easier to read and we test each functions with the same input values
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] xvade commented on pull request #1878: CASSANDRA-17221: Math fcts
Posted by GitBox <gi...@apache.org>.
xvade commented on PR #1878:
URL: https://github.com/apache/cassandra/pull/1878#issuecomment-1296042655
Done, I updated the tests to look more like what you had in mind.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1878: CASSANDRA-17221: Math fcts
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1878:
URL: https://github.com/apache/cassandra/pull/1878#discussion_r1001247204
##########
src/java/org/apache/cassandra/cql3/functions/MathFcts.java:
##########
@@ -0,0 +1,140 @@
+ /*
+ * 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.cassandra.cql3.functions;
+
+ import java.nio.ByteBuffer;
+ import java.util.Collection;
+ import java.util.List;
+ import java.util.stream.Collectors;
+
+ import com.google.common.collect.ImmutableList;
+ import org.slf4j.Logger;
+ import org.slf4j.LoggerFactory;
+
+ import org.apache.cassandra.db.marshal.*;
+ import org.apache.cassandra.transport.ProtocolVersion;
+
+ public class MathFcts
Review Comment:
Good point, I think we should probably also name it something like `MathFcts**Util**` similar to others like `SyncUtil`, etc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] XV4DE commented on a diff in pull request #1878: CASSANDRA-17221: Math fcts
Posted by GitBox <gi...@apache.org>.
XV4DE commented on code in PR #1878:
URL: https://github.com/apache/cassandra/pull/1878#discussion_r1008770540
##########
src/java/org/apache/cassandra/cql3/functions/MathFcts.java:
##########
@@ -0,0 +1,140 @@
+ /*
+ * 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.cassandra.cql3.functions;
+
+ import java.nio.ByteBuffer;
+ import java.util.Collection;
+ import java.util.List;
+ import java.util.stream.Collectors;
+
+ import com.google.common.collect.ImmutableList;
+ import org.slf4j.Logger;
+ import org.slf4j.LoggerFactory;
+
+ import org.apache.cassandra.db.marshal.*;
+ import org.apache.cassandra.transport.ProtocolVersion;
+
+ public class MathFcts
Review Comment:
Ok, I made it final, added a private constructor, and renamed it to `MathFctsUtil`, I also changed the test class name to match.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-17221: Math fcts [cassandra]
Posted by "pmcfadin (via GitHub)" <gi...@apache.org>.
pmcfadin closed pull request #1878: CASSANDRA-17221: Math fcts
URL: https://github.com/apache/cassandra/pull/1878
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on pull request #1878: Math fcts
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on PR #1878:
URL: https://github.com/apache/cassandra/pull/1878#issuecomment-1256900058
hi @XV4DE , what is the jira ticket this patch is for? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blerer commented on a diff in pull request #1878: CASSANDRA-17221: Math fcts
Posted by GitBox <gi...@apache.org>.
blerer commented on code in PR #1878:
URL: https://github.com/apache/cassandra/pull/1878#discussion_r981375482
##########
test/unit/org/apache/cassandra/cql3/functions/MathFctsTest.java:
##########
@@ -0,0 +1,231 @@
+/*
+ * 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.cassandra.cql3.functions;
+
+import java.nio.ByteBuffer;
+import java.util.Collections;
+import java.util.List;
+
+import org.junit.Test;
+
+import org.apache.cassandra.db.marshal.CounterColumnType;
+import org.apache.cassandra.db.marshal.DoubleType;
+import org.apache.cassandra.db.marshal.FloatType;
+import org.apache.cassandra.db.marshal.IntegerType;
+import org.apache.cassandra.db.marshal.ByteType;
+import org.apache.cassandra.db.marshal.DecimalType;
+import org.apache.cassandra.db.marshal.Int32Type;
+import org.apache.cassandra.db.marshal.LongType;
+import org.apache.cassandra.db.marshal.ShortType;
+import org.apache.cassandra.transport.ProtocolVersion;
+import org.apache.cassandra.utils.ByteBufferUtil;
+
+import static org.junit.Assert.assertEquals;
+
+public class MathFctsTest
Review Comment:
It does not seems to me that we really test properly the functions. The set of input we use is pretty limited and the test inputs are different with different types assuming that the type is not an important part of the testing.
I would make sense to test each types with the same input. It looks like it should be possible to create some test utility methods that take a type, an input and an expected output to validate a function.
##########
src/java/org/apache/cassandra/cql3/functions/MathFcts.java:
##########
@@ -0,0 +1,140 @@
+ /*
+ * 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.cassandra.cql3.functions;
+
+ import java.nio.ByteBuffer;
+ import java.util.Collection;
+ import java.util.List;
+ import java.util.stream.Collectors;
+
+ import com.google.common.collect.ImmutableList;
+ import org.slf4j.Logger;
+ import org.slf4j.LoggerFactory;
+
+ import org.apache.cassandra.db.marshal.*;
+ import org.apache.cassandra.transport.ProtocolVersion;
+
+ public class MathFcts
Review Comment:
Nits: As the class contains only static methods it should be final and the constructor should be private.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] xvade commented on pull request #1878: CASSANDRA-17221: Math fcts
Posted by GitBox <gi...@apache.org>.
xvade commented on PR #1878:
URL: https://github.com/apache/cassandra/pull/1878#issuecomment-1263047033
> I feel that the patch is nearly ready. The main issue for me, outside of the fact that the new dependency go lost along the way, is that the tests are a bit limited.
Hi @blerer, what are you looking for in the tests that you don't see? Is there a class I could model off of to improve them?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #1878: CASSANDRA-17221: Math fcts
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #1878:
URL: https://github.com/apache/cassandra/pull/1878#discussion_r1001247204
##########
src/java/org/apache/cassandra/cql3/functions/MathFcts.java:
##########
@@ -0,0 +1,140 @@
+ /*
+ * 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.cassandra.cql3.functions;
+
+ import java.nio.ByteBuffer;
+ import java.util.Collection;
+ import java.util.List;
+ import java.util.stream.Collectors;
+
+ import com.google.common.collect.ImmutableList;
+ import org.slf4j.Logger;
+ import org.slf4j.LoggerFactory;
+
+ import org.apache.cassandra.db.marshal.*;
+ import org.apache.cassandra.transport.ProtocolVersion;
+
+ public class MathFcts
Review Comment:
Good point, I think we should probably also name it something like `MathFctsUtil` similar to others like `SyncUtil`, etc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org