Merge pull request ClickHouse#62287 from ClickHouse/vdimir/internal_functions_fix

Fix __actionName, add tests for internal functions direct call

Author: vdimir

src/Functions/identity.h

@@ -6,6 +6,11 @@
 namespace DB
 {
 
+namespace ErrorCodes
+{
+    extern const int BAD_ARGUMENTS;
+}
+
 template<typename Name>
 class FunctionIdentityBase : public IFunction
 {
@@ -53,7 +58,24 @@ class FunctionActionName : public FunctionIdentityBase<ActionNameName>
     using FunctionIdentityBase::FunctionIdentityBase;
     static FunctionPtr create(ContextPtr) { return std::make_shared<FunctionActionName>(); }
     size_t getNumberOfArguments() const override { return 2; }
-    ColumnNumbers getArgumentsThatAreAlwaysConstant() const override { return {1}; }
+    ColumnNumbers getArgumentsThatAreAlwaysConstant() const override { return {0, 1}; }
+
+    /// Do not allow any argument to have type other than String
+    bool useDefaultImplementationForNulls() const override { return false; }
+    bool useDefaultImplementationForNothing() const override { return false; }
+    bool useDefaultImplementationForLowCardinalityColumns() const override { return false; }
+
+    DataTypePtr getReturnTypeImpl(const DataTypes & arguments) const override
+    {
+        for (const auto & arg : arguments)
+        {
+            if (WhichDataType(arg).isString())
+                continue;
+            throw Exception(ErrorCodes::BAD_ARGUMENTS, "Function __actionName is internal nad should not be used directly");
+        }
+
+        return FunctionIdentityBase<ActionNameName>::getReturnTypeImpl(arguments);
+    }
 };
 
 }

src/Planner/PlannerActionsVisitor.cpp

@@ -160,7 +160,17 @@ class ActionNodeNameHelper
                 const auto & function_node = node->as<FunctionNode &>();
                 if (function_node.getFunctionName() == "__actionName")
                 {
-                    result = toString(function_node.getArguments().getNodes().at(1)->as<ConstantNode>()->getValue());
+                    /// Perform sanity check, because user may call this function with unexpected arguments
+                    const auto & function_argument_nodes = function_node.getArguments().getNodes();
+                    if (function_argument_nodes.size() == 2)
+                    {
+                        if (const auto * second_argument = function_argument_nodes.at(1)->as<ConstantNode>())
+                            result = toString(second_argument->getValue());
+                    }
+
+                    /// Empty node name is not allowed and leads to logical errors
+                    if (result.empty())
+                        throw Exception(ErrorCodes::BAD_ARGUMENTS, "Function __actionName is internal nad should not be used directly");
                     break;
                 }
 

src/Processors/Transforms/MergeJoinTransform.cpp

@@ -338,6 +338,8 @@ static void prepareChunk(Chunk & chunk)
 
 void MergeJoinAlgorithm::initialize(Inputs inputs)
 {
+    LOG_DEBUG(&Poco::Logger::get("XXXX"), "{}:{}: {} - '{}'", __FILE__, __LINE__, 0, inputs[0].chunk.dumpStructure());
+    LOG_DEBUG(&Poco::Logger::get("XXXX"), "{}:{}: {} - '{}'", __FILE__, __LINE__, 1, inputs[1].chunk.dumpStructure());
     if (inputs.size() != 2)
         throw Exception(ErrorCodes::LOGICAL_ERROR, "Two inputs are required, got {}", inputs.size());
 
@@ -349,6 +351,8 @@ void MergeJoinAlgorithm::initialize(Inputs inputs)
 
 void MergeJoinAlgorithm::consume(Input & input, size_t source_num)
 {
+    LOG_DEBUG(&Poco::Logger::get("XXXX"), "{}:{}: {} - '{}'", __FILE__, __LINE__, source_num, input.chunk.dumpStructure());
+
     if (input.skip_last_row)
         throw Exception(ErrorCodes::NOT_IMPLEMENTED, "skip_last_row is not supported");
 
@@ -812,8 +816,17 @@ IMergingAlgorithm::Status MergeJoinAlgorithm::merge()
     if (!cursors[1]->cursor.isValid() && !cursors[1]->fullyCompleted())
         return Status(1);
 
+    for (size_t i = 0; i < 2; ++i)
+    {
+        LOG_DEBUG(&Poco::Logger::get("XXXX"), "{}:{}: sampleColumns {} '{}'", __FILE__, __LINE__, i, cursors[i]->sampleBlock().dumpStructure());
+    }
+
+
     if (auto result = handleAllJoinState())
+    {
+        LOG_DEBUG(&Poco::Logger::get("XXXX"), "{}:{}: '{}'", __FILE__, __LINE__, result ? result->chunk.dumpStructure() : "NA");
         return std::move(*result);
+    }
 
     if (cursors[0]->fullyCompleted() || cursors[1]->fullyCompleted())
     {

tests/queries/0_stateless/03035_internal_functions_direct_call.reference

@@ -0,0 +1,5 @@
+aaa
+(1,1)	(1,1)
+1
+a1	1
+1

tests/queries/0_stateless/03035_internal_functions_direct_call.sql

@@ -0,0 +1,25 @@
+-- This functions should not be called directly, only for internal use.
+-- However, we cannot completely forbid it (becasue query can came from another server, for example)
+-- Check that usage of these functions does not lead to crash or logical error
+
+SELECT __actionName(); -- { serverError NUMBER_OF_ARGUMENTS_DOESNT_MATCH }
+SELECT __actionName('aaa', 'aaa', 'aaa'); -- { serverError NUMBER_OF_ARGUMENTS_DOESNT_MATCH }
+SELECT __actionName('aaa', '') SETTINGS allow_experimental_analyzer = 1; -- { serverError BAD_ARGUMENTS }
+SELECT __actionName('aaa', materialize('aaa')); -- { serverError BAD_ARGUMENTS,ILLEGAL_COLUMN }
+SELECT __actionName(materialize('aaa'), 'aaa'); -- { serverError ILLEGAL_COLUMN }
+SELECT __actionName('aaa', 'aaa');
+
+SELECT concat(__actionName('aaa', toNullable('x')), '1') GROUP BY __actionName('aaa', 'x'); -- { serverError BAD_ARGUMENTS }
+
+SELECT __getScalar('aaa'); -- { serverError BAD_ARGUMENTS }
+SELECT __getScalar(); -- { serverError NUMBER_OF_ARGUMENTS_DOESNT_MATCH }
+SELECT __getScalar(1); -- { serverError ILLEGAL_TYPE_OF_ARGUMENT }
+SELECT __getScalar(materialize('1')); -- { serverError ILLEGAL_TYPE_OF_ARGUMENT }
+
+WITH ( SELECT (1,1) ) as a SELECT materialize(a), __getScalar('17789833925953107877_7493841889429261611') SETTINGS allow_experimental_analyzer = 1;
+
+SELECT __scalarSubqueryResult('1');
+SELECT 'a' || __scalarSubqueryResult(a), materialize('1') as a;
+SELECT __scalarSubqueryResult(a, a), materialize('1') as a; -- { serverError NUMBER_OF_ARGUMENTS_DOESNT_MATCH }
+
+SELECT 1 as `__grouping_set`;