You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metamodel.apache.org by ardlema <gi...@git.apache.org> on 2015/07/27 11:36:02 UTC
[GitHub] metamodel pull request: Refactoring to support scalar function
GitHub user ardlema opened a pull request:
https://github.com/apache/metamodel/pull/34
Refactoring to support scalar function
The current code only supports aggregate functions. In order to support more type of functions such as scalar we need to do some refactor.
The changes made in this PR include:
- The FunctionType is now an interface which has static final references to the aggregate functions.
- New AggregateFunction interface that contains the aggregation specific methods
- New DefaultAggregateFunction which implements the AggregateFunction interface
- ScalarFunction interface which is an skeleton where we should add the new scalar functions
- FunctionTypeFactory that creates AggregateFunctions from its name
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ardlema/metamodel METAMODEL-165
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metamodel/pull/34.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 #34
----
commit ba6a04fa2bd00697c1ce2dfde3eb0332711cf24e
Author: Alberto Rodriguez <ar...@stratio.com>
Date: 2015-07-27T08:41:23Z
Refactoring to generalize functions support
commit 9301a8e6f833985e80e4978008249c7d7d308279
Author: Alberto Rodriguez <ar...@stratio.com>
Date: 2015-07-27T09:14:01Z
Fixed failing tests: build method returns now a new instance of aggregate builders
commit b053029f1167a8c9977aa1d1b5edd1c245e754d4
Author: Alberto Rodriguez <ar...@stratio.com>
Date: 2015-07-27T09:26:36Z
Added some JavaDoc
----
---
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] metamodel pull request: Refactoring to support scalar function
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metamodel/pull/34
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/34#issuecomment-132332638
I also added METAMODEL-178 which this fixes.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36646483
--- Diff: core/src/main/java/org/apache/metamodel/query/DefaultAggregateFunction.java ---
@@ -0,0 +1,63 @@
+/**
+ * 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.metamodel.query;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.apache.metamodel.util.AggregateBuilder;
+
+/**
+ * Implementation of the {@link org.apache.metamodel.query.AggregateFunction}.
+ */
+public abstract class DefaultAggregateFunction<T> implements AggregateFunction {
+
+ public abstract AggregateBuilder<T> createAggregateBuilder();
+
+ public abstract ColumnType getExpectedColumnType(ColumnType type);
--- End diff --
This method is being used in the getExpectedColumnType of SelectItem. I'd declare it as part of the AggregateFunction interface and then implement it in the DefaultAggregateFunction class.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/34#issuecomment-129258186
Added a bunch of new remarks. I have been pretty meticulous in checking the PR mostly because it is an API change, so I hope it's encouraging and does not feel like critisism. It looks quite good now, and if you agree on the remarks I have left then I will give it my +1 for sure :)
---
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] metamodel pull request: Refactoring to support scalar function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on the pull request:
https://github.com/apache/metamodel/pull/34#issuecomment-128405600
@kaspersorensen changes addressed
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/34#issuecomment-125328236
Very shallow review from my phone (on holiday :-)) - wouldn't it be better with one implementation for each function, e.g. Sum, avg etc., instead of one implementation to rule all (and thus limit the feeling of pluggability)
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36036345
--- Diff: core/src/main/java/org/apache/metamodel/query/FunctionType.java ---
@@ -18,105 +18,22 @@
*/
package org.apache.metamodel.query;
-import org.apache.metamodel.schema.Column;
-import org.apache.metamodel.schema.ColumnType;
import org.apache.metamodel.util.AggregateBuilder;
/**
- * Represents an aggregate function to use in a SelectItem.
- *
+ * Represents a generic function to use in a SelectItem.
+ *
* @see SelectItem
- */
-public enum FunctionType {
-
- COUNT {
- @Override
- public AggregateBuilder<Long> build() {
- return new CountAggregateBuilder();
- }
- },
- AVG {
- @Override
- public AggregateBuilder<Double> build() {
- return new AverageAggregateBuilder();
- }
- },
- SUM {
- @Override
- public AggregateBuilder<Double> build() {
- return new SumAggregateBuilder();
- }
- },
- MAX {
- @Override
- public AggregateBuilder<Object> build() {
- return new MaxAggregateBuilder();
- }
- },
- MIN {
- @Override
- public AggregateBuilder<Object> build() {
- return new MinAggregateBuilder();
- }
- };
-
- public ColumnType getExpectedColumnType(ColumnType type) {
- switch (this) {
- case COUNT:
- return ColumnType.BIGINT;
- case AVG:
- case SUM:
- return ColumnType.DOUBLE;
- case MAX:
- case MIN:
- return type;
- default:
- return type;
- }
- }
-
- public SelectItem createSelectItem(Column column) {
- return new SelectItem(this, column);
- }
-
- public SelectItem createSelectItem(String expression, String alias) {
- return new SelectItem(this, expression, alias);
- }
-
- public Object evaluate(Iterable<?> values) {
- AggregateBuilder<?> builder = build();
- for (Object object : values) {
- builder.add(object);
- }
- return builder.getAggregate();
- }
+*/
+public interface FunctionType extends Function {
- /**
- * Executes the function
- *
- * @param values
- * the values to be evaluated. If a value is null it won't be
- * evaluated
- * @return the result of the function execution. The return type class is
- * dependent on the FunctionType and the values provided. COUNT
- * yields a Long, AVG and SUM yields Double values and MAX and MIN
- * yields the type of the provided values.
- */
- public Object evaluate(Object... values) {
- AggregateBuilder<?> builder = build();
- for (Object object : values) {
- builder.add(object);
- }
- return builder.getAggregate();
- }
+ public static final AggregateFunction COUNT = new CountAggregateFunction();
+ public static final AggregateFunction AVG = new AverageAggregateFunction();
+ public static final AggregateFunction SUM = new SumAggregateFunction();
+ public static final AggregateFunction MAX = new MaxAggregateFunction();
+ public static final AggregateFunction MIN = new MinAggregateFunction();
- public abstract AggregateBuilder<?> build();
+ public AggregateBuilder<?> build();
--- End diff --
I think this method is implementation oriented and should maybe then be kept out of the interface. Or else the evaluate method should - they kinda serve the same purpose.
And they are both aggregation function oriented so none of them should be here but if anywhere in the AggregateFunction interface.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36594696
--- Diff: core/src/main/java/org/apache/metamodel/query/FunctionTypeFactory.java ---
@@ -0,0 +1,43 @@
+/**
+ * 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.metamodel.query;
+
+/**
+ * Factory to create AggregateFunctions through
+ * its function name.
+ *
+ */
+public class FunctionTypeFactory {
+
+ public static AggregateFunction get(String functionName) {
+ if (functionName.equals("COUNT")) {
+ return new CountAggregateFunction();
+ } else if (functionName.equals("AVG")) {
+ return new AverageAggregateFunction();
+ } else if (functionName.equals("SUM")) {
+ return new SumAggregateFunction();
+ } else if (functionName.equals("MAX")) {
+ return new MaxAggregateFunction();
+ } else if (functionName.equals("MIN")) {
+ return new MinAggregateFunction();
--- End diff --
I guess it would be nicer to just return the single instances that we know from FunctionType.MIN, FunctionType.COUNT here etc...
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36206643
--- Diff: core/src/main/java/org/apache/metamodel/query/FunctionType.java ---
@@ -18,105 +18,22 @@
*/
package org.apache.metamodel.query;
-import org.apache.metamodel.schema.Column;
-import org.apache.metamodel.schema.ColumnType;
import org.apache.metamodel.util.AggregateBuilder;
/**
- * Represents an aggregate function to use in a SelectItem.
- *
+ * Represents a generic function to use in a SelectItem.
+ *
* @see SelectItem
- */
-public enum FunctionType {
-
- COUNT {
- @Override
- public AggregateBuilder<Long> build() {
- return new CountAggregateBuilder();
- }
- },
- AVG {
- @Override
- public AggregateBuilder<Double> build() {
- return new AverageAggregateBuilder();
- }
- },
- SUM {
- @Override
- public AggregateBuilder<Double> build() {
- return new SumAggregateBuilder();
- }
- },
- MAX {
- @Override
- public AggregateBuilder<Object> build() {
- return new MaxAggregateBuilder();
- }
- },
- MIN {
- @Override
- public AggregateBuilder<Object> build() {
- return new MinAggregateBuilder();
- }
- };
-
- public ColumnType getExpectedColumnType(ColumnType type) {
- switch (this) {
- case COUNT:
- return ColumnType.BIGINT;
- case AVG:
- case SUM:
- return ColumnType.DOUBLE;
- case MAX:
- case MIN:
- return type;
- default:
- return type;
- }
- }
-
- public SelectItem createSelectItem(Column column) {
- return new SelectItem(this, column);
- }
-
- public SelectItem createSelectItem(String expression, String alias) {
- return new SelectItem(this, expression, alias);
- }
-
- public Object evaluate(Iterable<?> values) {
- AggregateBuilder<?> builder = build();
- for (Object object : values) {
- builder.add(object);
- }
- return builder.getAggregate();
- }
+*/
+public interface FunctionType extends Function {
- /**
- * Executes the function
- *
- * @param values
- * the values to be evaluated. If a value is null it won't be
- * evaluated
- * @return the result of the function execution. The return type class is
- * dependent on the FunctionType and the values provided. COUNT
- * yields a Long, AVG and SUM yields Double values and MAX and MIN
- * yields the type of the provided values.
- */
- public Object evaluate(Object... values) {
- AggregateBuilder<?> builder = build();
- for (Object object : values) {
- builder.add(object);
- }
- return builder.getAggregate();
- }
+ public static final AggregateFunction COUNT = new CountAggregateFunction();
+ public static final AggregateFunction AVG = new AverageAggregateFunction();
+ public static final AggregateFunction SUM = new SumAggregateFunction();
+ public static final AggregateFunction MAX = new MaxAggregateFunction();
+ public static final AggregateFunction MIN = new MinAggregateFunction();
- public abstract AggregateBuilder<?> build();
+ public AggregateBuilder<?> build();
--- End diff --
But I would still retain the existing method too. Just have the two new getters implemented to check if function is instance of e.g. AggregateFunction and then return it if so.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36036364
--- Diff: core/src/main/java/org/apache/metamodel/query/DefaultAggregateFunction.java ---
@@ -0,0 +1,64 @@
+/**
+ * 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.metamodel.query;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.apache.metamodel.util.AggregateBuilder;
+
+/**
+ * Implementation of the {@link org.apache.metamodel.query.AggregateFunction}.
+ */
+public abstract class DefaultAggregateFunction<T> {
+
+ String functionType;
--- End diff --
This field isn't so nicely exposed IMO. It's both mutable and unencapsulated. So it requires quite some studying by the subclass author to understand its meaning.
I could imagine it better using an abstract method or as a private final field that is set via the constructor.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36183060
--- Diff: core/src/main/java/org/apache/metamodel/query/FunctionType.java ---
@@ -18,105 +18,22 @@
*/
package org.apache.metamodel.query;
-import org.apache.metamodel.schema.Column;
-import org.apache.metamodel.schema.ColumnType;
import org.apache.metamodel.util.AggregateBuilder;
/**
- * Represents an aggregate function to use in a SelectItem.
- *
+ * Represents a generic function to use in a SelectItem.
+ *
* @see SelectItem
- */
-public enum FunctionType {
-
- COUNT {
- @Override
- public AggregateBuilder<Long> build() {
- return new CountAggregateBuilder();
- }
- },
- AVG {
- @Override
- public AggregateBuilder<Double> build() {
- return new AverageAggregateBuilder();
- }
- },
- SUM {
- @Override
- public AggregateBuilder<Double> build() {
- return new SumAggregateBuilder();
- }
- },
- MAX {
- @Override
- public AggregateBuilder<Object> build() {
- return new MaxAggregateBuilder();
- }
- },
- MIN {
- @Override
- public AggregateBuilder<Object> build() {
- return new MinAggregateBuilder();
- }
- };
-
- public ColumnType getExpectedColumnType(ColumnType type) {
- switch (this) {
- case COUNT:
- return ColumnType.BIGINT;
- case AVG:
- case SUM:
- return ColumnType.DOUBLE;
- case MAX:
- case MIN:
- return type;
- default:
- return type;
- }
- }
-
- public SelectItem createSelectItem(Column column) {
- return new SelectItem(this, column);
- }
-
- public SelectItem createSelectItem(String expression, String alias) {
- return new SelectItem(this, expression, alias);
- }
-
- public Object evaluate(Iterable<?> values) {
- AggregateBuilder<?> builder = build();
- for (Object object : values) {
- builder.add(object);
- }
- return builder.getAggregate();
- }
+*/
+public interface FunctionType extends Function {
- /**
- * Executes the function
- *
- * @param values
- * the values to be evaluated. If a value is null it won't be
- * evaluated
- * @return the result of the function execution. The return type class is
- * dependent on the FunctionType and the values provided. COUNT
- * yields a Long, AVG and SUM yields Double values and MAX and MIN
- * yields the type of the provided values.
- */
- public Object evaluate(Object... values) {
- AggregateBuilder<?> builder = build();
- for (Object object : values) {
- builder.add(object);
- }
- return builder.getAggregate();
- }
+ public static final AggregateFunction COUNT = new CountAggregateFunction();
+ public static final AggregateFunction AVG = new AverageAggregateFunction();
+ public static final AggregateFunction SUM = new SumAggregateFunction();
+ public static final AggregateFunction MAX = new MaxAggregateFunction();
+ public static final AggregateFunction MIN = new MinAggregateFunction();
- public abstract AggregateBuilder<?> build();
+ public AggregateBuilder<?> build();
--- End diff --
How about we add two methods to selectitem: getAggregateFunction and getScalarFunction. In Selectitem we can then do the instanceof check and otherwise return null.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36174387
--- Diff: core/src/main/java/org/apache/metamodel/query/DefaultAggregateFunction.java ---
@@ -0,0 +1,64 @@
+/**
+ * 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.metamodel.query;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.apache.metamodel.util.AggregateBuilder;
+
+/**
+ * Implementation of the {@link org.apache.metamodel.query.AggregateFunction}.
+ */
+public abstract class DefaultAggregateFunction<T> {
+
+ String functionType;
--- End diff --
You're absolutely right, I'll modify it
---
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] metamodel pull request: Refactoring to support scalar function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36609619
--- Diff: core/src/main/java/org/apache/metamodel/query/ScalarFunction.java ---
@@ -0,0 +1,25 @@
+/**
+ * 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.metamodel.query;
+
+
+public interface ScalarFunction extends FunctionType {
--- End diff --
Completely agree
---
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] metamodel pull request: Refactoring to support scalar function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on the pull request:
https://github.com/apache/metamodel/pull/34#issuecomment-125519871
@kaspersorensen I've created an implementation for each function
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36589963
--- Diff: core/src/main/java/org/apache/metamodel/query/AverageAggregateFunction.java ---
@@ -0,0 +1,35 @@
+/**
+ * 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.metamodel.query;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.apache.metamodel.util.AggregateBuilder;
+
+public class AverageAggregateFunction extends DefaultAggregateFunction<Double> implements AggregateFunction {
+
+ public String getFunctionName() { return "AVG"; }
--- End diff --
Normally we always have "regular" line breaks inside the brackets. This also applies to the other implementations which has the same code style pattern in this regard.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36589948
--- Diff: core/src/main/java/org/apache/metamodel/query/AggregateFunction.java ---
@@ -0,0 +1,33 @@
+/**
+ * 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.metamodel.query;
+
+import org.apache.metamodel.util.AggregateBuilder;
+
+/**
+ * Interface that contains the aggregation specific methods
+ * related to the AggregateBuilder.
+ *
+ */
+public interface AggregateFunction extends FunctionType {
+
+ public AggregateBuilder<?> createAggregateBuilder();
+
+ public Object evaluate(Object... values);
--- End diff --
Not blocking for me right now (since we can easily add it), but I would prefer some javadoc on those methods. Especially to also point out that the evaluate method is just a shorthand for creating an aggregate builder, adding all the values and then calculating the value. Maybe that even warrants removing the method, but maybe you don't want to do that as part of this refactoring.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36609548
--- Diff: core/src/main/java/org/apache/metamodel/query/AggregateFunction.java ---
@@ -0,0 +1,33 @@
+/**
+ * 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.metamodel.query;
+
+import org.apache.metamodel.util.AggregateBuilder;
+
+/**
+ * Interface that contains the aggregation specific methods
+ * related to the AggregateBuilder.
+ *
+ */
+public interface AggregateFunction extends FunctionType {
+
+ public AggregateBuilder<?> createAggregateBuilder();
+
+ public Object evaluate(Object... values);
--- End diff --
I'll add it as part of this PR
---
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] metamodel pull request: Refactoring to support scalar function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36185192
--- Diff: core/src/main/java/org/apache/metamodel/query/FunctionType.java ---
@@ -18,105 +18,22 @@
*/
package org.apache.metamodel.query;
-import org.apache.metamodel.schema.Column;
-import org.apache.metamodel.schema.ColumnType;
import org.apache.metamodel.util.AggregateBuilder;
/**
- * Represents an aggregate function to use in a SelectItem.
- *
+ * Represents a generic function to use in a SelectItem.
+ *
* @see SelectItem
- */
-public enum FunctionType {
-
- COUNT {
- @Override
- public AggregateBuilder<Long> build() {
- return new CountAggregateBuilder();
- }
- },
- AVG {
- @Override
- public AggregateBuilder<Double> build() {
- return new AverageAggregateBuilder();
- }
- },
- SUM {
- @Override
- public AggregateBuilder<Double> build() {
- return new SumAggregateBuilder();
- }
- },
- MAX {
- @Override
- public AggregateBuilder<Object> build() {
- return new MaxAggregateBuilder();
- }
- },
- MIN {
- @Override
- public AggregateBuilder<Object> build() {
- return new MinAggregateBuilder();
- }
- };
-
- public ColumnType getExpectedColumnType(ColumnType type) {
- switch (this) {
- case COUNT:
- return ColumnType.BIGINT;
- case AVG:
- case SUM:
- return ColumnType.DOUBLE;
- case MAX:
- case MIN:
- return type;
- default:
- return type;
- }
- }
-
- public SelectItem createSelectItem(Column column) {
- return new SelectItem(this, column);
- }
-
- public SelectItem createSelectItem(String expression, String alias) {
- return new SelectItem(this, expression, alias);
- }
-
- public Object evaluate(Iterable<?> values) {
- AggregateBuilder<?> builder = build();
- for (Object object : values) {
- builder.add(object);
- }
- return builder.getAggregate();
- }
+*/
+public interface FunctionType extends Function {
- /**
- * Executes the function
- *
- * @param values
- * the values to be evaluated. If a value is null it won't be
- * evaluated
- * @return the result of the function execution. The return type class is
- * dependent on the FunctionType and the values provided. COUNT
- * yields a Long, AVG and SUM yields Double values and MAX and MIN
- * yields the type of the provided values.
- */
- public Object evaluate(Object... values) {
- AggregateBuilder<?> builder = build();
- for (Object object : values) {
- builder.add(object);
- }
- return builder.getAggregate();
- }
+ public static final AggregateFunction COUNT = new CountAggregateFunction();
+ public static final AggregateFunction AVG = new AverageAggregateFunction();
+ public static final AggregateFunction SUM = new SumAggregateFunction();
+ public static final AggregateFunction MAX = new MaxAggregateFunction();
+ public static final AggregateFunction MIN = new MinAggregateFunction();
- public abstract AggregateBuilder<?> build();
+ public AggregateBuilder<?> build();
--- End diff --
That approach was my first thought but there are a lot of calls to the getFunction method scattered all around the code (FetchSizeCalculator, JdbcDataSet, QueryPostProcessDataContext, ConvertedDataSetInterceptor...). If we get rid of the getFunction method we should modify a lot of classes and ideally I do not want to do it for this refactoring :(
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36590013
--- Diff: core/src/main/java/org/apache/metamodel/query/parser/SelectItemParser.java ---
@@ -113,10 +110,10 @@ public SelectItem findSelectItem(String expression) throws MultipleSelectItemsPa
final int startParenthesis = expression.indexOf('(');
if (startParenthesis > 0 && expression.endsWith(")")) {
String functionName = expression.substring(0, startParenthesis);
- function = FunctionType.get(functionName.toUpperCase());
+ function = FunctionTypeFactory.get(functionName.toUpperCase());
if (function != null) {
expression = expression.substring(startParenthesis + 1, expression.length() - 1).trim();
- if (function == FunctionType.COUNT && "*".equals(expression)) {
+ if (function instanceof CountAggregateFunction && "*".equals(expression)) {
--- End diff --
We actually have SelectItem.isCountAllItem(...) to do this - maybe better to reuse.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36400984
--- Diff: core/src/main/java/org/apache/metamodel/query/DefaultAggregateFunction.java ---
@@ -0,0 +1,65 @@
+/**
+ * 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.metamodel.query;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.apache.metamodel.util.AggregateBuilder;
+
+/**
+ * Implementation of the {@link org.apache.metamodel.query.AggregateFunction}.
+ */
+public abstract class DefaultAggregateFunction<T> {
+
+ public abstract AggregateBuilder<T> build();
+
+ public abstract ColumnType getExpectedColumnType(ColumnType type);
+
+ public Object evaluate(Iterable<?> values) {
+ AggregateBuilder<?> builder = build();
+ for (Object object : values) {
+ builder.add(object);
+ }
+ return builder.getAggregate();
+ }
+
+ /**
+ * Executes the function
+ *
+ * @param values
+ * the values to be evaluated. If a value is null it won't be
+ * evaluated
+ * @return the result of the function execution. The return type class is
+ * dependent on the FunctionType and the values provided. COUNT
+ * yields a Long, AVG and SUM yields Double values and MAX and MIN
+ * yields the type of the provided values.
+ */
+ public Object evaluate(Object... values) {
+ AggregateBuilder<?> builder = build();
+ for (Object object : values) {
+ builder.add(object);
+ }
+ return builder.getAggregate();
+ }
+
+ public abstract String getFunctionType();
--- End diff --
This should be defined in the FunctionType interface, and I would give it the name GetFunctionName.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/34#issuecomment-129579864
Ok I am now 100 happy with the PR :-)
Since this change will break runtime compatibility (but NOT compile time compatibility, which is good) I think it warrants a bump in versioning to make this into MM 4.4.0. Then we can also stop supporting Java 6 in that version.
So actually I would prefer not to merge it to master just yet, but get a final MM 4.3.x release done first. Maybe start a vote on that Alberto?
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36589971
--- Diff: core/src/main/java/org/apache/metamodel/query/DefaultAggregateFunction.java ---
@@ -0,0 +1,63 @@
+/**
+ * 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.metamodel.query;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.apache.metamodel.util.AggregateBuilder;
+
+/**
+ * Implementation of the {@link org.apache.metamodel.query.AggregateFunction}.
+ */
+public abstract class DefaultAggregateFunction<T> implements AggregateFunction {
+
+ public abstract AggregateBuilder<T> createAggregateBuilder();
+
+ public abstract ColumnType getExpectedColumnType(ColumnType type);
+
+ public Object evaluate(Iterable<?> values) {
+ AggregateBuilder<?> builder = createAggregateBuilder();
+ for (Object object : values) {
+ builder.add(object);
+ }
+ return builder.getAggregate();
+ }
+
+ /**
+ * Executes the function
+ *
+ * @param values
+ * the values to be evaluated. If a value is null it won't be
+ * evaluated
+ * @return the result of the function execution. The return type class is
+ * dependent on the FunctionType and the values provided. COUNT
+ * yields a Long, AVG and SUM yields Double values and MAX and MIN
+ * yields the type of the provided values.
+ */
+ public Object evaluate(Object... values) {
+ AggregateBuilder<?> builder = createAggregateBuilder();
+ for (Object object : values) {
+ builder.add(object);
+ }
+ return builder.getAggregate();
+ }
+
+ @Override
+ public String toString() { return getFunctionName(); }
--- End diff --
and here too :)
---
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] metamodel pull request: Refactoring to support scalar function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on the pull request:
https://github.com/apache/metamodel/pull/34#issuecomment-129722132
Sounds like a plan. Looking forward to getting rid of Java 6 support ;)
Would you mind starting the vote?
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r35730748
--- Diff: core/src/main/java/org/apache/metamodel/query/parser/SelectItemParser.java ---
@@ -113,10 +110,10 @@ public SelectItem findSelectItem(String expression) throws MultipleSelectItemsPa
final int startParenthesis = expression.indexOf('(');
if (startParenthesis > 0 && expression.endsWith(")")) {
String functionName = expression.substring(0, startParenthesis);
- function = FunctionType.get(functionName.toUpperCase());
+ function = FunctionTypeFactory.get(functionName.toUpperCase());
if (function != null) {
expression = expression.substring(startParenthesis + 1, expression.length() - 1).trim();
- if (function == FunctionType.COUNT && "*".equals(expression)) {
+ if (function.toString().equals("COUNT") && "*".equals(expression)) {
--- End diff --
IMO it is better to compare the function object than comparing tostring. And I think we discussed having the builtin functions on the FunctionType interface still.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36674649
--- Diff: core/src/main/java/org/apache/metamodel/query/parser/SelectItemParser.java ---
@@ -113,10 +110,10 @@ public SelectItem findSelectItem(String expression) throws MultipleSelectItemsPa
final int startParenthesis = expression.indexOf('(');
if (startParenthesis > 0 && expression.endsWith(")")) {
String functionName = expression.substring(0, startParenthesis);
- function = FunctionType.get(functionName.toUpperCase());
+ function = FunctionTypeFactory.get(functionName.toUpperCase());
if (function != null) {
expression = expression.substring(startParenthesis + 1, expression.length() - 1).trim();
- if (function == FunctionType.COUNT && "*".equals(expression)) {
+ if (function instanceof CountAggregateFunction && "*".equals(expression)) {
--- End diff --
Good point, I missed that so I agree!
---
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] metamodel pull request: Refactoring to support scalar function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36609605
--- Diff: core/src/main/java/org/apache/metamodel/query/AverageAggregateFunction.java ---
@@ -0,0 +1,35 @@
+/**
+ * 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.metamodel.query;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.apache.metamodel.util.AggregateBuilder;
+
+public class AverageAggregateFunction extends DefaultAggregateFunction<Double> implements AggregateFunction {
+
+ public String getFunctionName() { return "AVG"; }
--- End diff --
Intellij faults ;) I'll add the line breaks. Good catch, thx!
---
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] metamodel pull request: Refactoring to support scalar function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on the pull request:
https://github.com/apache/metamodel/pull/34#issuecomment-125479911
@kaspersorensen Do you mean something like this?:
```
public class SumAggregateFunction extends DefaultAggregateFunction<Long> implements AggregateFunction {
public AggregateBuilder<Long> build() {
return new SumAggregateBuilder();
}
}
```
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/34#issuecomment-132331663
I'll merge it in now :)
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r35687584
--- Diff: core/src/main/java/org/apache/metamodel/query/AverageAggregateBuilder.java ---
@@ -23,7 +23,7 @@
/**
* Aggregate builder for the {@link FunctionType#AVG} function
*/
-final class AverageAggregateBuilder extends AbstractNumberAggregateBuilder<Double> {
+final class AverageAggregateBuilder<T> extends AbstractNumberAggregateBuilder<Double> {
--- End diff --
Why the new type parameter here?
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36400924
--- Diff: core/src/main/java/org/apache/metamodel/query/Function.java ---
@@ -0,0 +1,27 @@
+/**
+ * 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.metamodel.query;
+
+import org.apache.metamodel.schema.ColumnType;
+
+public interface Function {
--- End diff --
I would still delete this new class and just move the method (and a getFunctionName method for getting "AVG" etc.) to FunctionType.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by ardlema <gi...@git.apache.org>.
Github user ardlema commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r35732935
--- Diff: core/src/main/java/org/apache/metamodel/query/AverageAggregateBuilder.java ---
@@ -23,7 +23,7 @@
/**
* Aggregate builder for the {@link FunctionType#AVG} function
*/
-final class AverageAggregateBuilder extends AbstractNumberAggregateBuilder<Double> {
+final class AverageAggregateBuilder<T> extends AbstractNumberAggregateBuilder<Double> {
--- End diff --
Good catch! this is completely unnecessary, I'll get rid of it
---
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] metamodel pull request: Refactoring to support scalar function
Posted by ardlema <gi...@git.apache.org>.
Github user ardlema commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36165739
--- Diff: core/src/main/java/org/apache/metamodel/query/FunctionType.java ---
@@ -18,105 +18,22 @@
*/
package org.apache.metamodel.query;
-import org.apache.metamodel.schema.Column;
-import org.apache.metamodel.schema.ColumnType;
import org.apache.metamodel.util.AggregateBuilder;
/**
- * Represents an aggregate function to use in a SelectItem.
- *
+ * Represents a generic function to use in a SelectItem.
+ *
* @see SelectItem
- */
-public enum FunctionType {
-
- COUNT {
- @Override
- public AggregateBuilder<Long> build() {
- return new CountAggregateBuilder();
- }
- },
- AVG {
- @Override
- public AggregateBuilder<Double> build() {
- return new AverageAggregateBuilder();
- }
- },
- SUM {
- @Override
- public AggregateBuilder<Double> build() {
- return new SumAggregateBuilder();
- }
- },
- MAX {
- @Override
- public AggregateBuilder<Object> build() {
- return new MaxAggregateBuilder();
- }
- },
- MIN {
- @Override
- public AggregateBuilder<Object> build() {
- return new MinAggregateBuilder();
- }
- };
-
- public ColumnType getExpectedColumnType(ColumnType type) {
- switch (this) {
- case COUNT:
- return ColumnType.BIGINT;
- case AVG:
- case SUM:
- return ColumnType.DOUBLE;
- case MAX:
- case MIN:
- return type;
- default:
- return type;
- }
- }
-
- public SelectItem createSelectItem(Column column) {
- return new SelectItem(this, column);
- }
-
- public SelectItem createSelectItem(String expression, String alias) {
- return new SelectItem(this, expression, alias);
- }
-
- public Object evaluate(Iterable<?> values) {
- AggregateBuilder<?> builder = build();
- for (Object object : values) {
- builder.add(object);
- }
- return builder.getAggregate();
- }
+*/
+public interface FunctionType extends Function {
- /**
- * Executes the function
- *
- * @param values
- * the values to be evaluated. If a value is null it won't be
- * evaluated
- * @return the result of the function execution. The return type class is
- * dependent on the FunctionType and the values provided. COUNT
- * yields a Long, AVG and SUM yields Double values and MAX and MIN
- * yields the type of the provided values.
- */
- public Object evaluate(Object... values) {
- AggregateBuilder<?> builder = build();
- for (Object object : values) {
- builder.add(object);
- }
- return builder.getAggregate();
- }
+ public static final AggregateFunction COUNT = new CountAggregateFunction();
+ public static final AggregateFunction AVG = new AverageAggregateFunction();
+ public static final AggregateFunction SUM = new SumAggregateFunction();
+ public static final AggregateFunction MAX = new MaxAggregateFunction();
+ public static final AggregateFunction MIN = new MinAggregateFunction();
- public abstract AggregateBuilder<?> build();
+ public AggregateBuilder<?> build();
--- End diff --
I agree with you Kasper, we've got a problem in here though.
We are calling the build method from the MetaModelHelper class and this guy is picking up the FunctionType interface from the getFunction method of SelectItem. It means that if we move these methods to the AggregateFunction interface we should change the SelectItem class to make it implementation-oriented and then we need to duplicate constructor methods and have two variables for AggregateFunctionType and ScalarFunctionType interfaces.
Not sure how to solve this issue... any ideas?
---
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] metamodel pull request: Refactoring to support scalar function
Posted by ardlema <gi...@git.apache.org>.
Github user ardlema commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r35734602
--- Diff: core/src/main/java/org/apache/metamodel/query/parser/SelectItemParser.java ---
@@ -113,10 +110,10 @@ public SelectItem findSelectItem(String expression) throws MultipleSelectItemsPa
final int startParenthesis = expression.indexOf('(');
if (startParenthesis > 0 && expression.endsWith(")")) {
String functionName = expression.substring(0, startParenthesis);
- function = FunctionType.get(functionName.toUpperCase());
+ function = FunctionTypeFactory.get(functionName.toUpperCase());
if (function != null) {
expression = expression.substring(startParenthesis + 1, expression.length() - 1).trim();
- if (function == FunctionType.COUNT && "*".equals(expression)) {
+ if (function.toString().equals("COUNT") && "*".equals(expression)) {
--- End diff --
Change done!
---
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] metamodel pull request: Refactoring to support scalar function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36609637
--- Diff: core/src/main/java/org/apache/metamodel/query/parser/SelectItemParser.java ---
@@ -113,10 +110,10 @@ public SelectItem findSelectItem(String expression) throws MultipleSelectItemsPa
final int startParenthesis = expression.indexOf('(');
if (startParenthesis > 0 && expression.endsWith(")")) {
String functionName = expression.substring(0, startParenthesis);
- function = FunctionType.get(functionName.toUpperCase());
+ function = FunctionTypeFactory.get(functionName.toUpperCase());
if (function != null) {
expression = expression.substring(startParenthesis + 1, expression.length() - 1).trim();
- if (function == FunctionType.COUNT && "*".equals(expression)) {
+ if (function instanceof CountAggregateFunction && "*".equals(expression)) {
--- End diff --
Good point, I'll modify it. Thx!
---
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] metamodel pull request: Refactoring to support scalar function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on the pull request:
https://github.com/apache/metamodel/pull/34#issuecomment-127949679
@kaspersorensen I've added a new getter to SelectItem that will return an AggregateFunction interface. Could you please, check it out again? thx
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36400764
--- Diff: core/src/main/java/org/apache/metamodel/query/AggregateFunction.java ---
@@ -0,0 +1,35 @@
+/**
+ * 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.metamodel.query;
+
+import org.apache.metamodel.util.AggregateBuilder;
+
+/**
+ * Interface that contains the aggregation specific methods
+ * related to the AggregateBuilder.
+ *
+ */
+public interface AggregateFunction extends FunctionType {
+
+ Object evaluate(Iterable<?> values);
+
+ public AggregateBuilder<?> build();
+
+ public Object evaluate(Object... values);
--- End diff --
Sorry to keep bringing up these methods :-) seems the build method is the superior one, so I would keep only that. But rename it to something more explanatory, like createAggregateBuilder.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36589983
--- Diff: core/src/main/java/org/apache/metamodel/query/SelectItem.java ---
@@ -211,6 +211,13 @@ public FunctionType getFunction() {
return _function;
}
+ public AggregateFunction getAggregateFunction() {
+ if (_function instanceof AggregateFunction)
+ return (AggregateFunction) _function;
+ else
--- End diff --
I'm also missing some brackets here to comply with our normal code style.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/34#issuecomment-125858077
Looking at the Function and FunctionType interface I see too much overlap IMO. And methods like build and evaluate seem to only apply to aggregations. So maybe delete the new Function interface? And move methods to AggregateFunction interface?
Sorry for the very short remarks. On worst Internet connection ever.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by ardlema <gi...@git.apache.org>.
Github user ardlema commented on the pull request:
https://github.com/apache/metamodel/pull/34#issuecomment-125877219
@kaspersorensen changes addressed.
With regard to the Function interface I thought that this could be a good place to define all the generic methods for the different type of functions. Right now I agree with you: we can just move the getExpectedColumnType to the AggregateFunctionInterface but when we'd add the ScalarFunctionInterface we might end up duplicating generic functions in both interfaces.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36589989
--- Diff: core/src/main/java/org/apache/metamodel/query/ScalarFunction.java ---
@@ -0,0 +1,25 @@
+/**
+ * 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.metamodel.query;
+
+
+public interface ScalarFunction extends FunctionType {
--- End diff --
Javadoc would be good here. The word "scalar" is probably hard to digest for many people, so a bit explanation will 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] metamodel pull request: Refactoring to support scalar function
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on the pull request:
https://github.com/apache/metamodel/pull/34#issuecomment-129515764
@kaspersorensen Thank you for all the remarks, I've implemented all of them apart from the one related to the isCountAllItem function (see comment inline above).
As you said we are performing some important changes in the API so we must be careful with them.
---
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] metamodel pull request: Refactoring to support scalar function
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/34#discussion_r36590154
--- Diff: core/src/main/java/org/apache/metamodel/query/DefaultAggregateFunction.java ---
@@ -0,0 +1,63 @@
+/**
+ * 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.metamodel.query;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.apache.metamodel.util.AggregateBuilder;
+
+/**
+ * Implementation of the {@link org.apache.metamodel.query.AggregateFunction}.
+ */
+public abstract class DefaultAggregateFunction<T> implements AggregateFunction {
+
+ public abstract AggregateBuilder<T> createAggregateBuilder();
+
+ public abstract ColumnType getExpectedColumnType(ColumnType type);
--- End diff --
I cannot find out where this method is used for anything. So either I think we should not have it (will save a bunch of lines of code) or else it should maybe be part of the AggregateFunction interface?
---
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.
---