-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: use official math functions #619
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
Conversation
is the functionality exactly equivalent? Or will this break existing applications that use the old functions?
I compared the extension-functions.c
function definition with the official documentation. Which shows that it would definitely be a breaking change. It also shows that the current state of sql.js
is missing some SQL specified math functions. I didn't find any reported issues that noticed that the handful of math functions are missing.
missing in SQLITE_ENABLE_MATH_FUNCTIONS
aliases
atn2
(atan2
is official)square
(sqrt
is official)charindex
(instr
is official)
functionality
scalar
cot
coth
replicate
difference
sign
leftstr
rightstr
reverse
proper
padl
padr
padc
strfilter
aggregate
stdev
variance
mode
median
lower_quartile
upper_quartile
missing in extension-functions.c
aliases
ceiling
pow
functionality
ln
log(B,X)
(only haslog(X)
)log2
mod
trunc
Looks like extension functions.c is better, right?
Some of the missing features in MATH_FUNCTIONS would be hard to reproduce, whereas things like mod or log bases are easy to reproduce.
It greatly depends on the goal of the package. The name being sql.js
and not something containing SQLite
means that it is totally up to you to decide what is the correct behavior.
The trigger for me to make this PR is that we have a test suite that checks that our databases support the SQL specification. We currently are using better-sqlite3
for the node runtime, but would also like to support other runtimes by using sql.js
as a substitute. With these missing functions being the only gap in our test suite.
Probably the best solution would be to support all functions, but the feature flag and the extension define the same functions. I am not sure whether that will cause compilation problems. 🤷
It is also not even a show stopper for our project. As we know the difference in behavior between the implementations and can either opt for documenting the missing functions or decide to use create_function
to polyfill the missing functions.
Yes, I think create_function is the way to go for you ! It looks like it's much easier to fill the gap from extension-functions.c to SQLITE_ENABLE_MATH_FUNCTIONS than the opposite.
The currently used function extension file is listed by SQLite as "not supported nor maintained" and "Use At Your Own Risk" (docs).
The current feature flag
SQLITE_ENABLE_MATH_FUNCTIONS
is enabled automatically by newly generated Makefiles. Which provides an maintained and documented version of the math functions provided from the originally contributed functions extension.