I was looking through the function registery today and I noticed this:

    UDAFS_IMPLY_ORDER.add("rank");
    UDAFS_IMPLY_ORDER.add("dense_rank");
    UDAFS_IMPLY_ORDER.add("percent_rank");
    UDAFS_IMPLY_ORDER.add("cume_dist");
    UDAFS_IMPLY_ORDER.add(LEAD_FUNC_NAME);
    UDAFS_IMPLY_ORDER.add(LAG_FUNC_NAME);
    UDAFS_IMPLY_ORDER.add("first_value");
    UDAFS_IMPLY_ORDER.add("last_value");

There are several things to point out first.

1) I can not follow why some method names are constant and some are not.
2) A hard coded list of UDAFS_IMPLY_ORDER seems to indicate that a window
function of this type can not be added.
3) Every other container in function registry is a synchronized collection,
but this is not
-  /*
-   * UDAFS that only work when the input rows have an order.
-   */
-  public static final HashSet<String> UDAFS_IMPLY_ORDER = new
HashSet<String>();

4) Our style rules were not followed '{' sometimes on the line
sometimes tab, sometimes two spaces, sometimes 8 tabs,

  public static void registerTableFunction(String name, Class<? extends
TableFunctionResolver> tFnCls)
  {
    FunctionInfo tInfo = new FunctionInfo(name, tFnCls);
    mFunctions.put(name.toLowerCase(), tInfo);
  }

I have opened up https://issues.apache.org/jira/browse/HIVE-4879 to address
these things.

Personally I am not a crazy stickler for check-style and I rarely kick a
patch back at someone for a couple of style violations, but this really
needed the once over. Especially since we just spent a lot of effort
cleaning up un thread safe things so hiveserver2 can be happy.

Keep in mind that patches  with flagrant check style violations, no tests,
and little javadoc should be a no go.

I am adding an annotation for the classes so we do not need to maintain a
static list.
Edward

Reply via email to