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

Virtual columns #288

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@alekseymvt
Contributor

alekseymvt commented Jul 15, 2018

I think that I could add support for virtual columns (mysql/mariadb) and do not complicate the editing of the table.
Look here please.
I do not know whether I did everything correctly and did not spoil the editing of other types of databases.

v-columns

CREATE TABLE `my_table` (
  `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
  `date` date NOT NULL DEFAULT '2000-06-01',
  `json_data` text DEFAULT NULL,
  `user_name` varchar(25) GENERATED ALWAYS AS (concat_ws(' ',json_value(`json_data`,'$.first-name'),json_value(`json_data`,'$.last-name'))) STORED,
  `user_age` smallint(5) unsigned GENERATED ALWAYS AS (json_value(`json_data`,'$.about.age')) VIRTUAL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

INSERT INTO `my_table` (`id`, `date`, `json_data`) VALUES
(9,	'2000-01-01',	'{\"first-name\": \"John\",\"last-name\":\"Doe\",\"about\":{\"age\":27}}');

@alekseymvt alekseymvt force-pushed the alekseymvt:virtual_columns branch 2 times, most recently from 9426e99 to 000f502 Aug 18, 2018

@alekseymvt alekseymvt force-pushed the alekseymvt:virtual_columns branch from 000f502 to 3270fdb Sep 18, 2018

@alekseymvt

This comment has been minimized.

Contributor

alekseymvt commented Sep 18, 2018

Can you revert the unrelated changes (indentation and lines with just whitespace) and create a pull request?

I tried to fix it. I don't think I can more.

@vrana

I've decided to not accept this pull request, it has too many issues and unrelated changes. Also the change from SHOW FULL COLUMNS to SELECT * FROM information_schema.COLUMNS seems very risky.

@@ -44,13 +44,21 @@
if ($field["field"] != "") {
if (!$field["has_default"]) {
$field["default"] = null;
} elseif (($field["has_default"] == 'STORED') || ($field["has_default"] == 'VIRTUAL')) {
$field["virtual"] = "GENERATED ALWAYS AS ({$field['default']}) {$field['has_default']}";

This comment has been minimized.

@vrana

vrana Sep 19, 2018

Owner

Use "$field[default]", not "{$field['default']}". It's shorter and easier to read.

This comment has been minimized.

@alekseymvt

alekseymvt Sep 19, 2018

Contributor

Php manual: echo "This is wrong: $arr[foo]";
fix in new commits.

This comment has been minimized.

@vrana

vrana Sep 20, 2018

Owner

I co-created PHP manual and it's not true.

This comment has been minimized.

@alekseymvt

alekseymvt Sep 20, 2018

Contributor

http://php.net/manual/en/language.types.string.php

// This is wrong for the same reason as $foo[bar] is wrong  outside a string.
// In other words, it will still work, but only because PHP first looks for a
// constant named foo; an error of level E_NOTICE (undefined constant) will be
// thrown.
echo "This is wrong: {$arr[foo][3]}"; 

This comment has been minimized.

@vrana

vrana Sep 20, 2018

Owner

You need to read more carefully. $foo[bar] is wrong outside a string, not inside a string. "{$foo[bar]}" is also wrong but there's nothing wrong on "$foo[bar]" because bar is treated as a string, not as a constant.

$process_field = process_field($field, $type_field);
$all_fields[] = array($field["orig"], $process_field, $after);
if ($process_field != process_field($orig_field, $orig_field)) {
if ($process_field != process_field($orig_field, null)) {

This comment has been minimized.

@vrana

vrana Sep 19, 2018

Owner

Why this change?

This comment has been minimized.

@alekseymvt

alekseymvt Sep 19, 2018

Contributor

fix in new commits.

@@ -51,7 +51,7 @@ function result($query, $field = 0) {
$row = $result->fetch_array();
return $row[$field];
}

This comment has been minimized.

@vrana

vrana Sep 19, 2018

Owner

Avoid whitespace-only changes.

This comment has been minimized.

@alekseymvt

alekseymvt Sep 19, 2018

Contributor

IDE trim spaces

"type" => $match[1],
"length" => $match[2],
"unsigned" => ltrim($match[3] . $match[4]),
"default" => ($row["Default"] != "" || preg_match("~char|set~", $match[1]) ? $row["Default"] : null),

This comment has been minimized.

@vrana

vrana Sep 19, 2018

Owner

The preg_match("~char|set~", $match[1]) check disappeared. Why?

This comment has been minimized.

@alekseymvt

alekseymvt Sep 19, 2018

Contributor

I get "default" from schema

This comment has been minimized.

@vrana

vrana Sep 20, 2018

Owner

It was here probably for some reason (check blame to see it). This will probably break it. Please stop changing random stuff.

This comment has been minimized.

@alekseymvt

alekseymvt Sep 20, 2018

Contributor

Ok. But I don't know how to do it.

This comment has been minimized.

@vrana

vrana Sep 20, 2018

Owner

Then just keep it.

This comment has been minimized.

@alekseymvt

alekseymvt Sep 20, 2018

Contributor

fixed

"virtual" => isset($row["EXTRA"])
? ($row["EXTRA"] == "STORED GENERATED" ? 'STORED' : ($row["EXTRA"] == "VIRTUAL GENERATED" ? 'VIRTUAL' : false))
: false,
"expression" => isset($row['GENERATION_EXPRESSION']) ? $row['GENERATION_EXPRESSION'] : null

This comment has been minimized.

@vrana

vrana Sep 19, 2018

Owner

Avoid unnecessary isset().

This comment has been minimized.

@alekseymvt

alekseymvt Sep 19, 2018

Contributor

fix in new commits.

@@ -1422,6 +1422,8 @@ function edit_form($TABLE, $fields, $row, $update) {
echo "<table cellspacing='0'>" . script("qsl('table').onkeydown = editingKeydown;");
foreach ($fields as $name => $field) {
if (!empty($field['virtual'])) continue;

This comment has been minimized.

@vrana

vrana Sep 19, 2018

Owner

Always use {} for blocks.

This comment has been minimized.

@alekseymvt

alekseymvt Sep 19, 2018

Contributor

fix in new commits.

@@ -1422,6 +1422,8 @@ function edit_form($TABLE, $fields, $row, $update) {
echo "<table cellspacing='0'>" . script("qsl('table').onkeydown = editingKeydown;");
foreach ($fields as $name => $field) {
if (!empty($field['virtual'])) continue;

This comment has been minimized.

@vrana

vrana Sep 19, 2018

Owner

Avoid empty().

This comment has been minimized.

@alekseymvt

alekseymvt Sep 19, 2018

Contributor

fix in new commits.

)); ?></td>
<td id="label-default"><?php echo lang('Default value'); ?></td>
<?php if (support("comment")) { ?>
<td id='label-comment' class="<?=$comments ? "" : "hidden" ?>"><?php echo lang('Comment')?></td>

This comment has been minimized.

@vrana

vrana Sep 19, 2018

Owner

Avoid <?=, it's not supported in all PHP versions. This change is also unnecessary.

This comment has been minimized.

@alekseymvt

alekseymvt Sep 19, 2018

Contributor

fix in new commits.

<option <?php echo $field["has_default"] ? 'selected' : '' ?> value="1">default
<?php if ($jush == 'sql') { ?>
<optgroup label="Virtual column">
<option <?php echo $field["virtual"] == 'STORED' ? 'selected' : '' ?> value="STORED">stored

This comment has been minimized.

@vrana

vrana Sep 19, 2018

Owner

Do not indent HTML.

This comment has been minimized.

@alekseymvt

alekseymvt Sep 19, 2018

Contributor

fix in new commits.

@@ -1437,7 +1439,7 @@ function edit_form($TABLE, $fields, $row, $update) {
)
: (!$update && $field["auto_increment"]
? ""
: (isset($_GET["select"]) ? false : $default)
: (isset($_GET["select"]) ? false : (empty($field["null"]) ? $default : null))

This comment has been minimized.

@vrana

vrana Sep 19, 2018

Owner

What's this?

This comment has been minimized.

@alekseymvt

alekseymvt Sep 19, 2018

Contributor

There was a small bug.
if field schema is NULL DEFAULT NULL then in insert new record "NULL" will be string(!) by default.

This comment has been minimized.

@vrana

vrana Sep 20, 2018

Owner

This should be resolved when displaying the default value in the form, not here.

This comment has been minimized.

@alekseymvt

alekseymvt Sep 20, 2018

Contributor

fix in new commits.

@vrana vrana closed this Sep 19, 2018

"virtual" => isset($row["EXTRA"])
? ($row["EXTRA"] == "STORED GENERATED" ? 'STORED' : ($row["EXTRA"] == "VIRTUAL GENERATED" ? 'VIRTUAL' : false))
: false,
"expression" => isset($row['GENERATION_EXPRESSION']) ? $row['GENERATION_EXPRESSION'] : null

This comment has been minimized.

@vrana

vrana Sep 19, 2018

Owner

Can you store this under the default key? It could simplify some other code.

This comment has been minimized.

@alekseymvt

alekseymvt Sep 19, 2018

Contributor

Changed in new commits.

@alekseymvt

This comment has been minimized.

Contributor

alekseymvt commented Sep 19, 2018

@vrana Better than this, I can not do.
master...alekseymvt:virtual_columns

@alekseymvt

This comment has been minimized.

Contributor

alekseymvt commented Sep 20, 2018

@vrana

This comment has been minimized.

Owner

vrana commented Sep 20, 2018

Resolve the remaining comments.

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