- 
                Notifications
    
You must be signed in to change notification settings  - Fork 25.6k
 
ESQL: Replace "representable" type error messages #131775
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 18 commits
6a4b727
              9a12405
              3cab2c1
              cd63166
              e5a0a78
              b9ff56f
              48cf5de
              82e84b1
              847bba8
              9a5c860
              861f108
              6886dff
              df624be
              12ccac0
              dffe6a6
              a8c7040
              896daad
              9e9161e
              28229a4
              f5233ac
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| pr: 131775 | ||
| summary: Replace "representable" type error messages | ||
| area: ES|QL | ||
| type: enhancement | ||
| issues: [] | 
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -77,9 +77,12 @@ public Count( | |
| "aggregate_metric_double", | ||
| "boolean", | ||
| "cartesian_point", | ||
| "cartesian_shape", | ||
| "date", | ||
| "date_nanos", | ||
| "double", | ||
| "geo_point", | ||
| "geo_shape", | ||
| 
         
      Comment on lines
    
      +80
     to 
      +85
    
   
  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing types for the docs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh. I thought the tests would catch that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mostly. We check that every type in tests appears there, but if it isn't tested, we do nothing. And Count didn't have "error types" tests to check that we tested all the types  | 
||
| "integer", | ||
| "ip", | ||
| "keyword", | ||
| 
          
            
          
           | 
    ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -37,6 +37,7 @@ | |
| import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.FIRST; | ||
| import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.SECOND; | ||
| import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isNotNullAndFoldable; | ||
| import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isRepresentableExceptCounters; | ||
| import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isType; | ||
| 
     | 
||
| public class Sample extends AggregateFunction implements ToAggregator { | ||
| 
        
          
        
         | 
    @@ -56,6 +57,7 @@ public class Sample extends AggregateFunction implements ToAggregator { | |
| "ip", | ||
| "keyword", | ||
| "long", | ||
| "unsigned_long", | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This trivially allowed unsigned longs, so just allowed it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mostly copied this from   | 
||
| "version" }, | ||
| description = "Collects sample values for a field.", | ||
| type = FunctionType.AGGREGATE, | ||
| 
        
          
        
         | 
    @@ -78,6 +80,7 @@ public Sample( | |
| "ip", | ||
| "keyword", | ||
| "long", | ||
| "unsigned_long", | ||
| "text", | ||
| "version" }, | ||
| description = "The field to collect sample values for." | ||
| 
          
            
          
           | 
    @@ -109,7 +112,7 @@ protected TypeResolution resolveType() { | |
| if (childrenResolved() == false) { | ||
| return new TypeResolution("Unresolved children"); | ||
| } | ||
| var typeResolution = isType(field(), dt -> dt != DataType.UNSIGNED_LONG, sourceText(), FIRST, "any type except unsigned_long").and( | ||
| var typeResolution = isRepresentableExceptCounters(field(), sourceText(), FIRST).and( | ||
                
       | 
||
| isNotNullAndFoldable(limitField(), sourceText(), SECOND) | ||
| ).and(isType(limitField(), dt -> dt == DataType.INTEGER, sourceText(), SECOND, "integer")); | ||
| if (typeResolution.unresolved()) { | ||
| 
          
            
          
           | 
    ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -44,6 +44,7 @@ public class Values extends AggregateFunction implements ToAggregator { | |
| private static final Map<DataType, Supplier<AggregatorFunctionSupplier>> SUPPLIERS = Map.ofEntries( | ||
| Map.entry(DataType.INTEGER, ValuesIntAggregatorFunctionSupplier::new), | ||
| Map.entry(DataType.LONG, ValuesLongAggregatorFunctionSupplier::new), | ||
| Map.entry(DataType.UNSIGNED_LONG, ValuesLongAggregatorFunctionSupplier::new), | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This trivially allowed unsigned longs, so just allowed it  | 
||
| Map.entry(DataType.DATETIME, ValuesLongAggregatorFunctionSupplier::new), | ||
| Map.entry(DataType.DATE_NANOS, ValuesLongAggregatorFunctionSupplier::new), | ||
| Map.entry(DataType.DOUBLE, ValuesDoubleAggregatorFunctionSupplier::new), | ||
| 
          
            
          
           | 
    @@ -72,6 +73,7 @@ public class Values extends AggregateFunction implements ToAggregator { | |
| "ip", | ||
| "keyword", | ||
| "long", | ||
| "unsigned_long", | ||
| "version" }, | ||
| preview = true, | ||
| appliesTo = { @FunctionAppliesTo(lifeCycle = FunctionAppliesToLifecycle.PREVIEW) }, | ||
| 
          
            
          
           | 
    @@ -111,6 +113,7 @@ public Values( | |
| "ip", | ||
| "keyword", | ||
| "long", | ||
| "unsigned_long", | ||
| "text", | ||
| "version" } | ||
| ) Expression v | ||
| 
          
            
          
           | 
    @@ -153,7 +156,7 @@ public DataType dataType() { | |
| 
     | 
||
| @Override | ||
| protected TypeResolution resolveType() { | ||
| return TypeResolutions.isType(field(), SUPPLIERS::containsKey, sourceText(), DEFAULT, "any type except unsigned_long"); | ||
| return TypeResolutions.isRepresentableExceptCounters(field(), sourceText(), DEFAULT); | ||
| } | ||
| 
     | 
||
| @Override | ||
| 
          
            
          
           | 
    ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some helpers to centralize the logic and messages here