Skip to content

math expression support#2090

Merged
fjy merged 3 commits intoapache:masterfrom
himanshug:math_exp
Apr 12, 2016
Merged

math expression support#2090
fjy merged 3 commits intoapache:masterfrom
himanshug:math_exp

Conversation

@himanshug
Copy link
Copy Markdown
Contributor

this PR adds a math expression implementation to be used in #1965, possibly as a post aggregator and some other places to query. I will send/update those PR once this one is merged.

pls see math-expr.md for the quick overview/documentation.

also, experimental DruidSQL.g4 is removed because it was failing with newer version of ANTLR, can bring it back and fix if/when needed.

@xvrl
Copy link
Copy Markdown
Member

xvrl commented Dec 14, 2015

@himanshug any reason we are rolling our own parser / lexer instead of levering antlr or something similar?

@himanshug
Copy link
Copy Markdown
Contributor Author

@xvrl mostly because parser/lexer is simple/small for this grammar (see the number of lines with real code in Lexer/Parser class), having it implemented here means less magic. All the rest of the code would be needed whether you used ANTLR or not.

@xvrl
Copy link
Copy Markdown
Member

xvrl commented Dec 14, 2015

@himanshug we already have a build-time dependency for antlr that's used for the toy sql grammar I wrote a while back. We could use antlr to remove all the boilerplate code and reduce the chance of bugs. I think it would make it easier to extend / understand for other folks, and remove a lot of boilerplate code.
Here's what the sql one looked like:
https://github.com/druid-io/druid/blob/master/server/src/main/antlr4/io/druid/sql/antlr4/DruidSQL.g4

Comment thread docs/content/misc/math-expr.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precedence is generally shown in decreasing order.
There's a missing "the" in front of "following operators".

@himanshug
Copy link
Copy Markdown
Contributor Author

@xvrl i agree with "understandability and extensibility" point, I will try to generate lexer/parser with antlr.

@himanshug himanshug force-pushed the math_exp branch 2 times, most recently from 91b6424 to b0aca0a Compare December 16, 2015 07:19
@himanshug
Copy link
Copy Markdown
Contributor Author

@xvrl generated stuff with ANTLR .
@rasahner comments addressed

Comment thread docs/content/misc/math-expr.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs "the" before "following" - sorry I didn't say that before.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

Comment thread docs/content/misc/math-expr.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be in query docs

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Feb 27, 2016

@himanshug we need docs on how to use this in queries

@himanshug
Copy link
Copy Markdown
Contributor Author

@fjy language introduced in this PR is not directly usable, it will be made availble to druid queries via #1965 and the doc for same will also appear there.

@jon-wei
Copy link
Copy Markdown
Contributor

jon-wei commented Mar 10, 2016

👍

This can be for later, but any thoughts on whether it's useful to expand this with more supported math expressions? e.g. trigonometric functions, ln, e^x?

@himanshug
Copy link
Copy Markdown
Contributor Author

@jon-wei yes, I will be adding more math functions, this PR sets up the framework really... adding more math functions would be trivial.

@navis
Copy link
Copy Markdown
Contributor

navis commented Mar 14, 2016

@himanshug Is it necessary to remove our toy-sql runner? :)

@himanshug
Copy link
Copy Markdown
Contributor Author

@navis had to remove that because it was written with an older version of antlr and was broken with antlr4. older version required writing grammar and implementation details in the same file which was messy.

@navis
Copy link
Copy Markdown
Contributor

navis commented Mar 22, 2016

👍 wanna use this.

@navis
Copy link
Copy Markdown
Contributor

navis commented Apr 7, 2016

@himanshug Could you rebase this? Let get this in.

@navis
Copy link
Copy Markdown
Contributor

navis commented Apr 8, 2016

@fjy The sole conflict is from DruidSql.g4, which was removed in this PR. I've asked @himanshug on it before but can we remove it and proceed?

@himanshug
Copy link
Copy Markdown
Contributor Author

@navis @fjy rebased to fix conflicts, will update #1965 once this one is merged to use this.

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Apr 11, 2016

@himanshug This needs docs

@himanshug
Copy link
Copy Markdown
Contributor Author

@fjy pls see #2090 (comment)

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Apr 12, 2016

👍

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Apr 12, 2016

@xvrl I can't not commit and squash?

@fjy fjy merged commit bd6bd34 into apache:master Apr 12, 2016
@fjy
Copy link
Copy Markdown
Contributor

fjy commented Apr 12, 2016

@xvrl nvm updated settings

@xvrl
Copy link
Copy Markdown
Member

xvrl commented Apr 13, 2016

@fjy from now on you just hit the "confirm squash and merge" button, I disabled the old merge as indicated in the mailing list.

@himanshug himanshug deleted the math_exp branch May 13, 2016 16:22
@giaosudau
Copy link
Copy Markdown
Contributor

This PR delete ExprLexer and ExprParser and those classes still a dependency of alot of components.
For example in: package io.druid.math.expr.Parser

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
import com.metamx.common.logger.Logger;
import io.druid.math.expr.antlr.ExprLexer;
import io.druid.math.expr.antlr.ExprParser;
import org.antlr.v4.runtime.ANTLRInputStream;
import org.antlr.v4.runtime.CommonTokenStream;
import org.antlr.v4.runtime.tree.ParseTree;
import org.antlr.v4.runtime.tree.ParseTreeWalker;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants