HPCC-13036 Add support for embed activities #11042

ghalliday · 2018-04-18T15:54:33Z

Signed-off-by: Gavin Halliday gavin.halliday@lexisnexis.com

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Testing:

hpcc-jirabot · 2018-04-18T15:54:42Z

https://track.hpccsystems.com/browse/HPCC-13036
Jira updated

ghalliday · 2018-04-18T16:01:25Z

@jakesmith @richardkchapman please review.
See also HPCC-19493 for issues relating to coding activities in python etc.

richardkchapman · 2018-04-23T13:00:41Z

@ghalliday Why the failures?

ghalliday · 2018-04-23T13:56:48Z

@AttilaVamos It wasn't very obvious from the smoke test results that I forgot to check in the key files.
@richardkchapman try again.

AttilaVamos · 2018-04-23T15:38:40Z

@ghalliday It is a bug in Regression Test Engine. I raised a JIRA to fix it.

ghalliday · 2018-04-25T11:03:05Z

@jakesmith @richardkchapman for review

richardkchapman · 2018-04-25T12:53:07Z

ecl/hqlcpp/hqlcppsys.ecl

@@ -790,6 +790,7 @@ const char * cppSystemText[]  = {
    "   boolean newMemorySpillSplitArg(unsigned4 usageCount, const varstring name, boolean meta) : include, pseudoentrypoint='new CLibraryMemorySpillSplitArg';",
    "   boolean newWorkUnitReadArg(const varstring _name, boolean _meta) : include, pseudoentrypoint='new CLibraryWorkUnitReadArg';",
    "   boolean newWorkUnitWriteArg(const varstring _name, unsigned4 _flags, boolean _meta) : include, pseudoentrypoint='new CLibraryWorkUnitWriteArg';",
+    "   CThorExternalArg(unsigned4 _numInputs) : include;",


I don't understand this line

nor do I. Is it for a forward declaration?

It is the declaration of the constructor for the helper class - so it can be called as a fake external function.

Perhaps a comment?

richardkchapman · 2018-04-25T12:56:31Z

rtl/include/eclhelper.hpp

+    virtual unsigned queryStrand() const = 0; // 0 based 0..numStrands-1
+};
+
+//MORE: How does is this extended to support onStart/onCReate


typo onCReate

richardkchapman · 2018-04-25T12:57:47Z

Minor comments - but not sure I would have spotted anything.

@jakesmith

jakesmith · 2018-04-25T17:00:17Z

ecl/hqlcpp/hqlcppsys.ecl

@@ -790,6 +790,7 @@ const char * cppSystemText[]  = {
    "   boolean newMemorySpillSplitArg(unsigned4 usageCount, const varstring name, boolean meta) : include, pseudoentrypoint='new CLibraryMemorySpillSplitArg';",
    "   boolean newWorkUnitReadArg(const varstring _name, boolean _meta) : include, pseudoentrypoint='new CLibraryWorkUnitReadArg';",
    "   boolean newWorkUnitWriteArg(const varstring _name, unsigned4 _flags, boolean _meta) : include, pseudoentrypoint='new CLibraryWorkUnitWriteArg';",
+    "   CThorExternalArg(unsigned4 _numInputs) : include;",


nor do I. Is it for a forward declaration?

jakesmith · 2018-04-25T17:04:00Z

ecl/hthor/hthor.ipp

+    virtual unsigned numSlaves() const { return 1; }
+    virtual unsigned numStrands() const { return strands; }
+    virtual unsigned querySlave() const { return 0; }
+    virtual unsigned queryStrand() const { return curStrand; }


above method could have 'override'

jakesmith · 2018-04-25T17:04:53Z

ecl/regress/embedactivity1err.ecl

+        }
+        RTLIMPLEMENT_IINTERFACE
+
+        virtual const void *nextRow()


could have override

jakesmith · 2018-04-25T17:05:02Z

ecl/regress/embedactivity1err.ecl

+            memcpy(row+sizeof(__uint64)+sizeof(size32_t), name, lenName);
+            return rowBuilder.finalizeRowClear(len);
+        }
+        virtual void stop()


could have override

jakesmith · 2018-04-25T17:06:10Z

roxie/ccd/ccdserver.cpp

+    virtual unsigned numSlaves() const { return 1; }
+    virtual unsigned numStrands() const { return strands; }
+    virtual unsigned querySlave() const { return 0; }
+    virtual unsigned queryStrand() const { return curStrand; }


above could be declared with 'override'

jakesmith · 2018-04-25T17:10:33Z

thorlcr/activities/external/thexternalslave.cpp

+// ExternalSlaveActivity
+//
+
+class NodeActivityContext : public IThorActivityContext


We should agree and try to be consistent, and I don't mind changing style, but all Thor classes begin with C, so e.g. CNodeActivityContext

I will change.
I think I use CX for a class that is the only concrete implementation of an interface. If there is more than one, then I often don't have a C prefix.

jakesmith · 2018-04-25T17:10:42Z

thorlcr/activities/external/thexternalslave.cpp

+    virtual unsigned numSlaves() const { return slaves; }
+    virtual unsigned numStrands() const { return 1; }
+    virtual unsigned querySlave() const { return curSlave; }
+    virtual unsigned queryStrand() const { return 0; }


above could be 'override'

jakesmith · 2018-04-25T17:12:04Z

thorlcr/activities/external/thexternalslave.cpp

+    virtual void process() override
+    {
+        start();
+        processed = 0;


no point in this line

jakesmith · 2018-04-25T17:12:52Z

thorlcr/activities/external/thexternalslave.ipp

@@ -0,0 +1,34 @@
+/*##############################################################################
+
+    HPCC SYSTEMS software Copyright (C) 2012 HPCC Systems®.


jakesmith · 2018-04-25T17:15:13Z

thorlcr/activities/external/thexternalslave.ipp

+
+
+activityslaves_decl CActivityBase *createExternalSlave(CGraphElementBase *container);
+activityslaves_decl CActivityBase *createExternalSinkSlave(CGraphElementBase *container);


because no one likes .ipp's and only here to include these factory methods and these are internal to this dll (activityslaves), I tend not to add a activity .ipp for activities and instead these declarations appear near top of slave.cpp (around line 244 onward)

jakesmith · 2018-04-25T17:15:56Z

@ghalliday - some minor comments.

ghalliday · 2018-04-26T16:58:19Z

@jakesmith please check revisions.

jakesmith · 2018-04-27T08:42:11Z

thorlcr/activities/external/thexternalslave.cpp

+    bool local;
+};
+
+class ExternalSlaveActivity : public CSlaveActivity


picky: you changed NodeActivityContext to CNodeActivityContext, would be good to change this and ExternalSinkSlaveActivity further down.

jakesmith · 2018-04-27T08:42:34Z

@ghalliday - 1 picky comment, but otherwise looks good.

ghalliday · 2018-04-27T15:32:00Z

@jakesmith renamed class and squashed.

jakesmith · 2018-04-27T16:31:00Z

@ghalliday - Looks good to merge

richardkchapman · 2018-04-30T07:22:04Z

ecl/hql/hqlattr.cpp

+        if (callIsActivity(expr))
+        {
+            //Can only be deduced by substituting the parameters into the body and seeing if the local attribute has a constant value
+            //currently assume false.


Is this something that needs improving?

richardkchapman · 2018-04-30T07:24:49Z

ecl/hql/hqlutil.cpp

@@ -8079,6 +8079,8 @@ class Vs6CppNameMangler
            mangled.append("PVIGlobalCodeContext@@");
        else if (body->hasAttribute(userMatchFunctionAtom))
            mangled.append("PVIMatchWalker@@");
+        if (functionBodyIsActivity(body))


Not familiar with the mangling schemes, but is it correct that this one is an if while the others are "else if" ?

Because the others seem to be exclusive options - e.g., see generateFunctionPrototype(). I was missing an entry for gcc name mangling.

richardkchapman · 2018-04-30T07:27:57Z

ecl/hqlcpp/hqlhtcpp.cpp

+    if (!implementationClassName && constructorArgs)
+    {
+        StringBuffer baseClassName;
+        baseClassName.append("CThor").append(activityArgName).append("Arg");


Is this calling the constructor mentioned in comments above? If so not sure how it would work unless activityArgName was "External".

It would only work if an entry was added for any other based class that required it.

richardkchapman · 2018-04-30T07:29:21Z

ecl/hqlcpp/hqlhtcpp.cpp

+            if (value->isConstant())
+                instance->isLocal = getBoolValue(value, false);
+            else
+                reportError(queryLocation(expr), ECODETEXT(HQLERR_AttributeXMustBeConstant), "LOCAL");


Should this be done in the semantic check phase?

I have moved it, although I'm not convinced it is better.

richardkchapman · 2018-04-30T07:33:28Z

@ghalliday Some questions. And you don't seem to have addressed Jake's comment about an incorrect copyright date.

ghalliday · 2018-04-30T10:17:14Z

@richardkchapman see changes.

richardkchapman · 2018-05-02T07:37:04Z

@ghalliday Do you know why failing?

ghalliday · 2018-05-02T13:14:37Z

@AttilaVamos I think this is smoke test failure. The branch runs fine on my machine.

Signed-off-by: Gavin Halliday <gavin.halliday@lexisnexis.com>

jpmcmu · 2018-05-02T14:09:12Z

ecl/hqlcpp/hqlhtcpp.cpp

+    HqlExprArray actuals;
+    ForEachChild(i, formals)
+    {
+        if (i < bound.ordinality())


@ghalliday Would this work as expected in the case where the dataset input was not first? Something like the following:
test(boolean testValue, streamed dataset(r) ds)

It works as expected - only leading streamed datasets are treated as input activities.
That should be documented - I added a comment on the documentation jira.

HPCCSmoketest · 2018-05-02T16:56:25Z

Automated Smoketest: ✅
OS: centos 7.2.1511 (Linux 3.10.0-327.28.3.el7.x86_64)
Sha: c0d0e2f
Build: success
Install hpccsystems-platform-community_6.5.0-trunk0.el7.x86_64.rpm
HPCC Start: OK

Unit tests result:

Test	total	passed
unittest	88	88
wutoolTest(Dali)	19	19
wutoolTest(Cassandra)	19	19

Regression test result:

phase	total	pass
setup (hthor)	11	11
setup (thor)	11	11
setup (roxie)	11	11
test (hthor)	780	780
test (thor)	711	711
test (roxie)	824	824

HPCC Stop: OK
HPCC Uninstall: OK
Time stats:

Prep time	Build time	Package time	Install time	Start time	Test time	Stop time	Summary
35 sec (00:00:35)	185 sec (00:03:05)	56 sec (00:00:56)	6 sec (00:00:06)	21 sec (00:00:21)	1183 sec (00:19:43)	16 sec (00:00:16)	1502 sec (00:25:02)

ghalliday force-pushed the issue13036 branch from be08eb7 to 4a21ba3 Compare April 23, 2018 13:55

richardkchapman reviewed Apr 25, 2018

View reviewed changes

jakesmith reviewed Apr 25, 2018

View reviewed changes

ghalliday force-pushed the issue13036 branch from 3e378bd to a07f167 Compare April 26, 2018 15:02

jakesmith reviewed Apr 27, 2018

View reviewed changes

ghalliday force-pushed the issue13036 branch from a07f167 to 1bd163a Compare April 27, 2018 15:31

ghalliday force-pushed the issue13036 branch from 1bd163a to 58c93d8 Compare April 27, 2018 15:32

richardkchapman reviewed Apr 30, 2018

View reviewed changes

ghalliday force-pushed the issue13036 branch 2 times, most recently from 854299f to b66afcb Compare April 30, 2018 10:23

HPCC-13036 Add support for embed activities

c0d0e2f

Signed-off-by: Gavin Halliday <gavin.halliday@lexisnexis.com>

ghalliday force-pushed the issue13036 branch from b66afcb to c0d0e2f Compare May 2, 2018 13:59

jpmcmu reviewed May 2, 2018

View reviewed changes

richardkchapman merged commit 8038bba into hpcc-systems:master May 2, 2018

		@@ -0,0 +1,34 @@
		/*##############################################################################

		HPCC SYSTEMS software Copyright (C) 2012 HPCC Systems®.



		activityslaves_decl CActivityBase createExternalSlave(CGraphElementBase container);
		activityslaves_decl CActivityBase createExternalSinkSlave(CGraphElementBase container);

HPCC-13036 Add support for embed activities #11042

HPCC-13036 Add support for embed activities #11042

Conversation

ghalliday commented Apr 18, 2018 • edited

Type of change:

Checklist:

Testing:

hpcc-jirabot commented Apr 18, 2018

ghalliday commented Apr 18, 2018

richardkchapman commented Apr 23, 2018

ghalliday commented Apr 23, 2018

AttilaVamos commented Apr 23, 2018

ghalliday commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardkchapman commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith commented Apr 25, 2018

ghalliday commented Apr 26, 2018

Choose a reason for hiding this comment

jakesmith commented Apr 27, 2018

ghalliday commented Apr 27, 2018

jakesmith commented Apr 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday Apr 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardkchapman commented Apr 30, 2018

ghalliday commented Apr 30, 2018

richardkchapman commented May 2, 2018

ghalliday commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HPCCSmoketest commented May 2, 2018

ghalliday commented Apr 18, 2018 •

edited

ghalliday Apr 30, 2018 •

edited