[GitHub] [incubator-druid] clintropolis commented on a change in pull request #8197: Add IPv4 druid expressions

[GitBox <gi...@apache.org>]

clintropolis commented on a change in pull request #8197: Add IPv4 druid expressions
URL: https://github.com/apache/incubator-druid/pull/8197#discussion_r309482129
 
 

 ##########
 File path: processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java
 ##########
 @@ -0,0 +1,198 @@
+/*
+ * 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.druid.query.expression;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.net.util.SubnetUtils;
+import org.apache.druid.java.util.common.IAE;
+import org.apache.druid.math.expr.Expr;
+import org.apache.druid.math.expr.ExprEval;
+import org.apache.druid.math.expr.ExprMacroTable;
+import org.apache.druid.math.expr.ExprType;
+
+import javax.annotation.Nonnull;
+import java.util.List;
+
+/**
+ * <pre>
+ * Implements an expression that checks if an IPv4 address belongs to a particular subnet.
+ *
+ * Expression signatures:
+ * - boolean ipv4address_match(string address, string subnet)
+ * - boolean ipv4address_match(string address, string subnet, boolean inclusive)
+ * - boolean ipv4address_match(long address, string subnet)
+ * - boolean ipv4address_match(long address, string subnet, boolean inclusive)
+ *
+ * Valid "address" argument formats are:
+ * - unsigned int long (e.g., 3232235521)
+ * - unsigned int string (e.g., "3232235521")
+ * - IPv4 address dotted-decimal notation string (e.g., "198.168.0.1")
+ * - IPv6 IPv4-mapped address string (e.g., "::ffff:192.168.0.1")
+ *
+ * The argument format for the "subnet" argument should be a literal in CIDR notation
+ * (e.g., "198.168.0.0/16").
+ *
+ * An optional "inclusive" argument should be a boolean literal (e.g., 1, 0, "true", or "false") that
+ * indicates whether the network and the broadcast addresses should be considered part of the
+ * subnet. When this argument is absent, its value defaults to "false".
+ *
+ * The overloaded signature allows applying the expression to a dimension with mixed string and long
+ * representations of IPv4 addresses.
+ * </pre>
+ *
+ * @see IPv4AddressParseExprMacro
+ * @see IPv4AddressStringifyExprMacro
+ */
+public class IPv4AddressMatchExprMacro implements ExprMacroTable.ExprMacro
 
 Review comment:
   >The purpose of ExprMacro is to provide a way to precompute certain things before doing the row-by-row evaluation, so IMO it does make more sense to use ExprMacro (which is designed for this) rather than hook into parse-time Function methods.
   
   Thanks for the explanation, :+1: I had stored away in my head for whatever reason that the main purpose of `ExprMacro` was so extensions could define expressions. I wonder if there is a way we could cut down the amount of code/test that needs to be added for `ExprMacro` that live in main Druid, because it does seem to take a lot more to add an `ExprMacro` compared to a `Function`, but I don't have any good ideas at the moment, nor am I sure that allowing `Parser.getFunction` to also take the list of args and allow initializing a `Function` when creating the `FunctionExpr` would be the appropriate.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org