Painless: Simplify Naming in Lookup Package #32177

jdconrad · 2018-07-18T20:21:36Z

This removes some extraneous naming syntax and makes clear the meaning of certain naming conventions without ambiguities (stricter) within the lookup package. Purely mechanical change. Note this does not cover a large portion of the PainlessLookupBuilder and PainlessLookup yet as there are several more follow up PRs for these incoming.

elasticmachine · 2018-07-18T20:21:38Z

Pinging @elastic/es-core-infra

rjernst

Looks good. A few small nits/suggestions.

rjernst · 2018-07-19T19:39:54Z

...les/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookupBuilder.java

-                throw new IllegalArgumentException(
-                        "must use only_fqn parameter on painless class [" + painlessClassName + "] with no package");
+        String javaClassName = clazz.getName();
+        String importedCanonicalClassName = javaClassName.substring(javaClassName.lastIndexOf('.') + 1).replace('$', '.');


I would simplify this to just "importedClassName". Any name used in code always has to be canonical (like in java).

I'm not sure it makes sense to remove the canonical. It is a canonical name because it lops off the package but still follows the syntax with '.' for inner classes. package.Class.InnerClass -> Class.InnerClass. A non-canonical name would be package.Class$InnerClass -> Class$InnerClass based on the formal java names. What do you think?

Ok sounds fine then.

rjernst · 2018-07-19T19:41:37Z

...les/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookupUtility.java

+ *     <li> - class/clazz       (Class)          - a painless class represented by a java class including def and excluding array
+ *                                                 types </li>
+ *
+ *     <li> - type              (Class)          - a painless type represented by a java class including def and array types </li>


I think think type (not the naming scheme, but the concept) should be explained outside of this list? It applies to eg javaType above as well.

rjernst · 2018-07-19T19:43:18Z

...les/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookupUtility.java

+ *     <li> - javaClassName     (String)         - the fully qualified java class name where '$' tokens represent inner classes excluding
+ *                                                 def and array types </li>
+ *
+ *     <li> - javaClass          (Class)         - a java class excluding def and array types </li>


The distinction of what "class" means (why it does not include def or arrays) should probably be explained before this list.

Added some more docs.

rjernst · 2018-07-19T20:02:52Z

...les/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookupUtility.java

-            return null;
-        } else if (anyType.isArray()) {
-            Class<?> anyTypeComponent = anyType.getComponentType();
+    public static Class<?> javaTypeToType(Class<?> javaType) {


I'm unsure what this means. Can you elaborate, maybe with a javadoc string?

Went through all the methods in PainlessLookupUtility and added some brief javadocs for each. Will need more passes, but it at least should clear up some of the more confusing aspects of some of these methods.

jdconrad · 2018-07-19T21:14:53Z

@rjernst Thanks for the review! Will commit once CI passes. Left one last question if you would please take a quick look.

This removes some extraneous naming syntax and makes clear the meaning of certain naming conventions without ambiguities (stricter) within the lookup package. Purely mechanical change. Note this does not cover a large portion of the PainlessLookupBuilder and PainlessLookup yet as there are several more follow up PRs for these incoming.

@return

* master: Painless: Simplify Naming in Lookup Package (#32177) Handle missing values in painless (#32207) add support for write index resolution when creating/updating documents (#31520) ECS Task IAM profile credentials ignored in repository-s3 plugin (#31864) Remove indication of future multi-homing support (#32187) Rest test - allow for snapshots to take 0 milliseconds Make x-pack-core generate a pom file Rest HL client: Add put watch action (#32026) Build: Remove pom generation for plugin zip files (#32180) Fix comments causing errors with Java 11 Fix rollup on date fields that don't support epoch_millis (#31890) Detect and prevent configuration that triggers a Gradle bug (#31912) [test] port linux package packaging tests (#31943) Revert "Introduce a Hashing Processor (#31087)" (#32178) Remove empty @return from JavaDoc Adjust SSLDriver behavior for JDK11 changes (#32145) [test] use randomized runner in packaging tests (#32109) Add support for field aliases. (#32172) Painless: Fix caching bug and clean up addPainlessClass. (#32142) Call setReferences() on custom referring tokenfilters in _analyze (#32157) Fix BwC Tests looking for UUID Pre 6.4 (#32158) Improve docs for search preferences (#32159) use before instead of onOrBefore Add more contexts to painless execute api (#30511) Add EC2 credential test for repository-s3 (#31918) A replica can be promoted and started in one cluster state update (#32042) Fix Java 11 javadoc compile problem Fix CP for namingConventions when gradle home has spaces (#31914) Fix `range` queries on `_type` field for singe type indices (#31756) [DOCS] Update TLS on Docker for 6.3 (#32114) ESIndexLevelReplicationTestCase doesn't support replicated failures but it's good to know what they are Remove versionType from translog (#31945) Switch distribution to new style Requests (#30595) Build: Skip jar tests if jar disabled Painless: Add PainlessClassBuilder (#32141) Build: Make additional test deps of check (#32015) Disable C2 from using AVX-512 on JDK 10 (#32138) Build: Move shadow customizations into common code (#32014) Painless: Fix Bug with Duplicate PainlessClasses (#32110) Remove empty @param from Javadoc Re-disable packaging tests on suse boxes Docs: Fix missing example script quote (#32010) [ML] Wait for aliases in multi-node tests (#32086) [ML] Move analyzer dependencies out of categorization config (#32123) Ensure to release translog snapshot in primary-replica resync (#32045) Handle TokenizerFactory TODOs (#32063) Relax TermVectors API to work with textual fields other than TextFieldType (#31915) Updates the build to gradle 4.9 (#32087) Mute :qa:mixed-cluster indices.stats/10_index/Index - all’ Check that client methods match API defined in the REST spec (#31825) Enable testing in FIPS140 JVM (#31666) Fix put mappings java API documentation (#31955) Add exclusion option to `keep_types` token filter (#32012) [Test] Modify assert statement for ssl handshake (#32072)

* es/6.x: (24 commits) Fix broken backport Switch full-cluster-restart to new style Requests (#32140) Fix multi level nested sort (#32204) MINOR: Remove unused `IndexDynamicSettings` (#32237) (#32248) [Tests] Remove QueryStringQueryBuilderTests#toQuery class assertions (#32236) Switch rolling restart to new style Requests (#32147) Enhance Parent circuit breaker error message (#32056) [ML] Use default request durability for .ml-state index (#32233) Enable testing in FIPS140 JVM (#31666) (#32231) Remove indices stats timeout from monitoring docs TESTS: Check for Netty resource leaks (#31861) (#32225) Rename ranking evaluation response section (#32166) Dependencies: Upgrade to joda time 2.10 (#32160) Backport SSL context names (#30953) to 6.x (#32223) Require Gradle 4.9 as minimum version (#32200) Detect old trial licenses and mimic behaviour (#32209) Painless: Simplify Naming in Lookup Package (#32177) add support for write index resolution when creating/updating documents (#31520) A replica can be promoted and started in one cluster state update (#32042) Rest test - allow for snapshots to take 0 milliseconds ...

Simplify naming scheme.

e72f350

jdconrad added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v7.0.0 >refactoring v6.4.0 labels Jul 18, 2018

jdconrad requested a review from rjernst July 18, 2018 20:21

jdconrad added 3 commits July 18, 2018 14:26

Quick fix.

0580465

More renaming.

53809f1

Merge branch 'master' into clean10

5466924

rjernst approved these changes Jul 19, 2018

View reviewed changes

Response to PR comments.

b653921

jdconrad mentioned this pull request Jul 19, 2018

Painless: Roadmap #31009

Closed

23 tasks

jdconrad merged commit c7a41c5 into elastic:master Jul 19, 2018

jdconrad added the backport pending label Jul 19, 2018

jdconrad removed the backport pending label Jul 20, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Painless: Simplify Naming in Lookup Package #32177

Painless: Simplify Naming in Lookup Package #32177

jdconrad commented Jul 18, 2018

elasticmachine commented Jul 18, 2018

rjernst left a comment

rjernst Jul 19, 2018

jdconrad Jul 19, 2018

rjernst Jul 19, 2018

rjernst Jul 19, 2018

jdconrad Jul 19, 2018

rjernst Jul 19, 2018

jdconrad Jul 19, 2018 •

edited

Loading

rjernst Jul 19, 2018

jdconrad Jul 19, 2018

jdconrad commented Jul 19, 2018

Painless: Simplify Naming in Lookup Package #32177

Painless: Simplify Naming in Lookup Package #32177

Conversation

jdconrad commented Jul 18, 2018

elasticmachine commented Jul 18, 2018

rjernst left a 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

jdconrad Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdconrad commented Jul 19, 2018

jdconrad Jul 19, 2018 •

edited

Loading